302092 - Bad variable naming (kSourceURLLength) for a variable that isn't a constant

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Daniel Atallah]

(17:07:09) timelyx: datallah: lastly, could you file a bug about kSourceURLLength
(17:07:13) timelyx: that variable is terribly named
(17:07:59) datallah: timelyx: the bug would be that it is misnamed?
(17:08:04) timelyx: yes
(17:08:26) timelyx: the 'k' is really wrong. dropping the k prefix and
lowercasing the next letter would be fine

Comment 1

[Daniel Atallah]

Attachment: rename kSourceURLLength variable to sourceURLLength since it isn't a constant

Comment 2

[timeless]

Comment on attachment 190459 [details] [diff] [review]
rename kSourceURLLength variable to sourceURLLength since it isn't a constant

>Index: nsDataObj.cpp
>@@ -1200,7 +1200,7 @@ nsDataObj :: BuildPlatformHTML ( const c
>-  const PRInt32 kSourceURLLength    = mSourceURL.Length();
this line should probably have one more space before the = sign since for some
reason the code is trying to keep them at the same position:
>+  const PRInt32 sourceURLLength    = mSourceURL.Length();
>   const PRInt32 kNumberLength       = strlen(numPlaceholder);
> 
>   const PRInt32 kTotalHeaderLen     = strlen(startHTMLPrefix) +

Comment 3

[[e-mail]]

Comment on attachment 190459 [details] [diff] [review]
rename kSourceURLLength variable to sourceURLLength since it isn't a constant

>-  const PRInt32 kSourceURLLength    = mSourceURL.Length();
>+  const PRInt32 sourceURLLength    = mSourceURL.Length();
>   const PRInt32 kNumberLength       = strlen(numPlaceholder);
> 
>   const PRInt32 kTotalHeaderLen     = strlen(startHTMLPrefix) +
Well if you're going to be picky then kTotalHeaderLen isn't a constant either,
and I don't see why you're still declaring it const. And all the strings should
be in arrays so you can use sizeof instead of strlen. Also you didn't keep the
= signs lined up.

Comment 4

[Daniel Atallah]

Attachment: Implementation of Neil's suggestions (and more)

After spending a ton of time trying to understand the various string types, I
came to the conclusion that at the very least the usage in this method wasn't
consistent.

I made all the "static" strings actually static to the method and also
pre-calculated the the static part of the header length.  If there is a more
preferred way to do this, I'd love to know.

Comment 5

[timeless]

Comment on attachment 190713 [details] [diff] [review]
Implementation of Neil's suggestions (and more)

>RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v
>diff -u -d -p -r1.68 nsDataObj.cpp

it seems like all these should be PRUint32 instead of PRInt32, no?

>+  static const PRInt32 kHeaderLen = sizeof(kStartHTMLPrefix) - 1 +
>+                                    sizeof(kEndHTMLPrefix) - 1 +
>+                                    sizeof(kStartFragPrefix) - 1 +
>+                                    sizeof(kEndFragPrefix) - 1 +
>+                                    sizeof(kEndFragTrailer) - 1 +
>+                                    (4 * (sizeof(kNumPlaceholder) - 1));
>+  PRInt32 totalHeaderLen          = kHeaderLen;
>+  PRInt32 startHTMLOffset = totalHeaderLen;
>   PRInt32 startFragOffset = startHTMLOffset
...
>   PRInt32 endFragOffset   = startFragOffset
...
>   PRInt32 endHTMLOffset   = endFragOffset

Comment 6

[[e-mail]]

Comment on attachment 190713 [details] [diff] [review]
Implementation of Neil's suggestions (and more)

>+  clipboardString.Append(kStartHTMLPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", startHTMLOffset));
> 
>+  clipboardString.Append(kEndHTMLPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", endHTMLOffset));
> 
>+  clipboardString.Append(kStartFragPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", startFragOffset));
> 
>+  clipboardString.Append(kEndFragPrefix);
>   clipboardString.Append(nsPrintfCString("%08u", endFragOffset));
I'm wondering whether this couldn't be done in one big nsPrintfCString.

>+  clipboardString.Append(kEndFragTrailer);
> 
>+  clipboardString.Append(kHtmlHeaderString);
>+  clipboardString.Append(kFragmentHeaderString);
Hmm... I suppose you could switch to #define and use clipboardString.AppendLiteral(kEndFragTrailer kHtmlHederString kFragmentHeaderString); but I guess that's less readable.

>+  clipboardString.Append(kTrailingString);
I'd consider using AppendLiteral anyway, except that it's not documented to work on static const char arrays...

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Daniel, were you still planning on working on this?

Comment 8

[Daniel Atallah]

(In reply to comment #7)
> Daniel, were you still planning on working on this?
> 

I haven't had a working build setup for this in a while, so if someone else wants to take care of this that'd be great.

Comment 9

[Ray Kraesig [:rkraesig]]

Attachment: Bug 302092 - Perform various minor modernizations r=handyman

Perform a straightforward rename: constants get a "k-" prefix; non-constants
have it removed. No functional changes.

Comment 10

[Ray Kraesig [:rkraesig]]

Attachment: Bug 302092 - [2/2] Modernization and cleanup r=handyman

Perform some additional cleanup to bring the code closer to modern
standards.

Depends on D138087

Comment 11

[Ray Kraesig [:rkraesig]]

Attachment: Bug 302092 - [3/2] Constexpr all the things r=handyman

Following the original diff [1], break the header into constexpr and
non-constant portions, and compute their length separately.

This will almost certainly be done by the compiler anyway.

(If approved, this commit will be squashed into the previous.)

[0] https://bug302092.bmoattachments.org/attachment.cgi?id=190713

Depends on D138088

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37fd03a3eebc
Perform various minor modernizations r=handyman

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/37fd03a3eebc