Bug 217583 – -Wsign-compare warnings in URL.cpp and URLParser.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Michael Catanzaro]

Integer literals are signed, so anything + 1 is signed:

[205/6600] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/URL.cpp.o
../../Source/WTF/wtf/URL.cpp: In member function ‘unsigned int WTF::URL::pathStart() const’:
../../Source/WTF/wtf/URL.cpp:110:15: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
  110 |     if (start == m_schemeEnd + 1
      |         ~~~~~~^~~~~~~~~~~~~~~~~~

[206/6600] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/URLParser.cpp.o
../../Source/WTF/wtf/URLParser.cpp: In member function ‘bool WTF::URLParser::needsNonSpecialDotSlash() const’:
../../Source/WTF/wtf/URLParser.cpp:2501:22: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
 2501 |         && pathStart == m_url.m_schemeEnd + 1
      |            ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~

We could use unsigned literals:

diff --git a/Source/WTF/wtf/URL.cpp b/Source/WTF/wtf/URL.cpp
index ae0324da9fc3..cf2d6bf3538c 100644
--- a/Source/WTF/wtf/URL.cpp
+++ b/Source/WTF/wtf/URL.cpp
@@ -107,7 +107,7 @@ bool URL::hasSpecialScheme() const
 unsigned URL::pathStart() const
 {
     unsigned start = m_hostEnd + m_portLength;
-    if (start == m_schemeEnd + 1
+    if (start == m_schemeEnd + 1u
         && start + 1 < m_string.length()
         && m_string[start] == '/' && m_string[start + 1] == '.')
         start += 2;
diff --git a/Source/WTF/wtf/URLParser.cpp b/Source/WTF/wtf/URLParser.cpp
index 8bbb2c0149ca..22db53f93761 100644
--- a/Source/WTF/wtf/URLParser.cpp
+++ b/Source/WTF/wtf/URLParser.cpp
@@ -2498,7 +2498,7 @@ bool URLParser::needsNonSpecialDotSlash() const
 {
     auto pathStart = m_url.m_hostEnd + m_url.m_portLength;
     return !m_urlIsSpecial
-        && pathStart == m_url.m_schemeEnd + 1
+        && pathStart == m_url.m_schemeEnd + 1u
         && pathStart + 1 < m_url.m_string.length()
         && m_url.m_string[pathStart] == '/'
         && m_url.m_string[pathStart + 1] == '/';

Or we could use casts:

diff --git a/Source/WTF/wtf/URL.cpp b/Source/WTF/wtf/URL.cpp
index ae0324da9fc3..b94e99759fb2 100644
--- a/Source/WTF/wtf/URL.cpp
+++ b/Source/WTF/wtf/URL.cpp
@@ -107,7 +107,7 @@ bool URL::hasSpecialScheme() const
 unsigned URL::pathStart() const
 {
     unsigned start = m_hostEnd + m_portLength;
-    if (start == m_schemeEnd + 1
+    if (start == static_cast<unsigned>(m_schemeEnd + 1)
         && start + 1 < m_string.length()
         && m_string[start] == '/' && m_string[start + 1] == '.')
         start += 2;
diff --git a/Source/WTF/wtf/URLParser.cpp b/Source/WTF/wtf/URLParser.cpp
index 8bbb2c0149ca..be7ab18c4ca3 100644
--- a/Source/WTF/wtf/URLParser.cpp
+++ b/Source/WTF/wtf/URLParser.cpp
@@ -2498,7 +2498,7 @@ bool URLParser::needsNonSpecialDotSlash() const
 {
     auto pathStart = m_url.m_hostEnd + m_url.m_portLength;
     return !m_urlIsSpecial
-        && pathStart == m_url.m_schemeEnd + 1
+        && pathStart == static_cast<unsigned>(m_url.m_schemeEnd + 1)
         && pathStart + 1 < m_url.m_string.length()
         && m_url.m_string[pathStart] == '/'
         && m_url.m_string[pathStart + 1] == '/';

No strong preference from me, but I'll attach a patch that uses casts since I think it's slightly more clear.

[Michael Catanzaro]

Created attachment 411060 [details]
Patch

[Darin Adler]

Comment on attachment 411060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411060&action=review

> Source/WTF/wtf/URL.cpp:110
> +    if (start == static_cast<unsigned>(m_schemeEnd + 1)

Doesn’t make sense to me. Isn’t m_schemeEnd an unsigned already? Is there something about being a bitfield that makes things become signed?

[Darin Adler]

Comment on attachment 411060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411060&action=review

>> Source/WTF/wtf/URL.cpp:110
>> +    if (start == static_cast<unsigned>(m_schemeEnd + 1)
> 
> Doesn’t make sense to me. Isn’t m_schemeEnd an unsigned already? Is there something about being a bitfield that makes things become signed?

Oh, you said that using "1" rather than "1U" makes it signed.

I prefer 1U over 1u and certainly over static_cast<unsigned>.

[Michael Catanzaro]

1U it is, then

[Michael Catanzaro]

Created attachment 411112 [details]
Patch

[Darin Adler]

Comment on attachment 411112 [details]
Patch

Thanks.

[EWS]

Committed r268362: <https://trac.webkit.org/changeset/268362>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411112 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70213551>