Closed Bug 450048 Opened 12 years ago Closed 12 years ago

"nsFind.cpp(961) : warning C4309: 'initializing' : truncation of constant value"

Categories

(Core Graveyard :: Embedding: APIs, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: sgautherie, Assigned: sgautherie)

Details

(Whiteboard: [warning fix checkin: comment 12])

Attachments

(1 file, 3 obsolete files)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080809180301 Minefield/3.1a2pre] (home, debug) (W2Ksp4)

{{
.../embedding/components/find/src/nsFind.cpp(961) : warning C4309: 'initializing' : truncation of constant value
}}
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080809180301 Minefield/3.1a2pre] (home, debug) (W2Ksp4)
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #333177 - Flags: review?(jst)
Attachment #333177 - Flags: superreview?(benjamin)
Attachment #333177 - Flags: review?(jst)
Attachment #333177 - Flags: review?(benjamin)
Attachment #333177 - Flags: superreview?(benjamin)
Attachment #333177 - Flags: superreview+
Attachment #333177 - Flags: review?(benjamin)
Attachment #333177 - Flags: review+
Flags: in-testsuite-
Keywords: checkin-needed
pushing to ssh://hg.mozilla.org/mozilla-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #333177 - Attachment description: (Av1) <nsFind.cpp> → (Av1) <nsFind.cpp> [Checkin: Comment 2]
(In reply to comment #2)
> pushing to ssh://hg.mozilla.org/mozilla-central/
> searching for changes
> remote: adding changesets
> remote: adding manifests
> remote: adding file changes
> remote: added 1 changesets with 1 changes to 1 files

and the changeset is 3d68e0eddc44
Uh.. no.  That patch is wrong, wrong, and wrong.  The code that calls StripChars on a non-ASCII char is wrong too, but this patch breaks the "ignore soft hyphens in the document" part.  Where they're a heck of a lot more likely than in the pattern!

Please to be backing this out, and landing tests that would have caught this problem before "fixing" it again.
Status: RESOLVED → REOPENED
Flags: in-testsuite- → in-testsuite?
Resolution: FIXED → ---
Oh, little did I know that this was more than quieting down a compiler warning :(
Keywords: checkin-needed
Whiteboard: [c-n: Backout comment 3 // Leave opened]
(In reply to comment #4)
> Uh.. no.  That patch is wrong, wrong, and wrong.  The code that calls

Sorry, I must have missed that second usage :-(

{{
960   // Ignore soft hyphens in the pattern  
961   static const char kShy[] = { CH_SHY, 0 };
962   patAutoStr.StripChars(kShy);

1148     PRUnichar c = (t2b ? t2b[findex] : CHAR_TO_UNICHAR(t1b[findex]));
...
1180     // ignore soft hyphens in the document
1181     if (c == CH_SHY)
1182       continue;
}}

> StripChars on a non-ASCII char is wrong too, but this patch breaks the "ignore

<http://mxr.mozilla.org/comm-central/search?string=StripChars&case=on>

Would have to look into this separately.

> soft hyphens in the document" part.
Attached patch (Av2) local cast only (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080819081634 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Instead of backing out patch Av1, I suggested to apply this "cumulative" patch:
1- Undo patch Av1.
2- Apply cast locally only.
Attachment #334787 - Flags: superreview?(jst)
Attachment #334787 - Flags: review?(jst)
Whiteboard: [c-n: Backout comment 3 // Leave opened] → [c-n: Backout comment 3 (or apply Av2) // Leave opened]
Comment on attachment 334787 [details] [diff] [review]
(Av2) local cast only

Looks good to me, but I'll let bz stamp this one since he spotted the problem with the initial patch.
Attachment #334787 - Flags: superreview?(jst)
Attachment #334787 - Flags: superreview?(bzbarsky)
Attachment #334787 - Flags: review?(jst)
Attachment #334787 - Flags: review?(bzbarsky)
Agreed a plan with Boris on IRC:

Av2 patch is "fine";
but an enhanced patch is wanted, and I'll try to prepare it (later !);
yet, if tree reopens before I succeed (or give up), Av1 patch backout is wanted !

Notes for the next patch:
{{
replace the 173 with ((PRUnichar)0xAD)

original fix should have broken searching in a document with soft hyphens
eg a document with "foo&shy;bar" and searching for "foobar"
}}

(Now, I have to figure out how to write my first test... :->)
Status: REOPENED → ASSIGNED
Whiteboard: [c-n: Backout comment 3 (or apply Av2) // Leave opened] → [c-n: Backout comment 3 // Leave opened]
Comment on attachment 334787 [details] [diff] [review]
(Av2) local cast only

s/173/((PRUnichar)173)/ and looks good.
Attachment #334787 - Flags: superreview?(bzbarsky)
Attachment #334787 - Flags: superreview+
Attachment #334787 - Flags: review?(bzbarsky)
Attachment #334787 - Flags: review+
I backed out the original patch.
Keywords: checkin-needed
Whiteboard: [c-n: Backout comment 3 // Leave opened]
Attachment #333177 - Attachment description: (Av1) <nsFind.cpp> [Checkin: Comment 2] → (Av1) <nsFind.cpp> [Backout: Comment 11]
Attachment #333177 - Attachment is obsolete: true
Attachment #334787 - Attachment is obsolete: true
Comment on attachment 334787 [details] [diff] [review]
(Av2) local cast only

Actually, comment 11
{
bzbarsky@mozilla.com
Mon Aug 25 10:22:45 2008 -0700
ca2acd6c2aa0
Boris Zbarsky — Back out bug 333177 "fix", since it would break searching across &shy;
}
(which comment had a wrong bug number)
fixed the warning,
without needing to modify line 961 ;->
Attached patch (Bv1) class/interface test (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080917032624 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)

(
This is my first test: please, double check.

I "copied" code from |_highlightDoc()| at
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#497
)

Tests the basic features.

How can |wordBreaker| be tested ? (See my TODO.)

http://mxr.mozilla.org/mozilla-central/source/embedding/components/find/public/nsIFind.idl?mark=54,65-66,70-71#43
I wonder about |May be null|: the code doesn't allow a |null| value; am I misunderstanding this documentation ?

NB: Boris, I'll file a separate bug about the StripChars issue, unless you can tell me more about it and it's not too hard (for me) to fix.
Attachment #339391 - Flags: superreview?(jst)
Attachment #339391 - Flags: review?(jst)
Attachment #339391 - Flags: review?(bzbarsky)
I'm probably not the best reviewer for this, especially for the test stuff.  I'll try to get to it if you don't find someone better (Neil Rashbrook, maybe?), but it'll take a while.
Sorry, I'm not really the best reviewer for tests...
Well, this needs review from someone familiar with how the nsIFind API is used.  I thought you were; if not, can you recommend someone?  I certainly am not.
Comment on attachment 339391 [details] [diff] [review]
(Bv1) class/interface test

Boris, I asked you (too) as you requested the test and I thought you would want to look at it.

From
http://hg.mozilla.org/mozilla-central/log/0de1994123e4/embedding/components/find/src/nsFind.cpp
jst should be just fine.
Attachment #339391 - Flags: review?(bzbarsky)
Comment on attachment 339391 [details] [diff] [review]
(Bv1) class/interface test

+  // TODO: I don't know how to set this :-(
+  // |Exception... "Cannot modify properties of a WrappedNative|
+  // rf.wordbreaker = null;

rf.wordBreaker (upper case 'B')?

r+sr=jst
Attachment #339391 - Flags: superreview?(jst)
Attachment #339391 - Flags: superreview+
Attachment #339391 - Flags: review?(jst)
Attachment #339391 - Flags: review+
Turns out the wordBreaker attribute is of type nsIWordBreaker, which is not an IDL interface at all. We should mark the attribute as [noscript] in the nsIFind.idl file while we're here, and not attempt to test anything related to it from JS as we simply can't.
Bv1, with interface update(s), approved by jst over irc.

http://hg.mozilla.org/mozilla-central/rev/d863285d0093
Attachment #339391 - Attachment is obsolete: true
(In reply to comment #13)
> I'll file a separate bug about the StripChars issue

I filed bug 456502.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite? → in-testsuite+
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [warning fix checkin: comment 12]
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.