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

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Serge Gautherie (:sgautherie)]

[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
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsFind.cpp> [Backout: Comment 11]

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

Comment 2

[Frank Wein [:mcsmurf]]

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

Comment 3

[Dão Gottwald [:dao]]

(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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Johnny Stenback (:jst)]

Oh, little did I know that this was more than quieting down a compiler warning :(

Comment 6

[Serge Gautherie (:sgautherie)]

(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.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) local cast only

[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.

Comment 8

[Johnny Stenback (:jst)]

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.

Comment 9

[Serge Gautherie (:sgautherie)]

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­bar" and searching for "foobar"
}}

(Now, I have to figure out how to write my first test... :->)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 334787 [details] [diff] [review]
(Av2) local cast only

s/173/((PRUnichar)173)/ and looks good.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I backed out the original patch.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 334787 [details] [diff] [review]
(Av2) local cast only

Actually, comment 11
{
[e-mail]
Mon Aug 25 10:22:45 2008 -0700
ca2acd6c2aa0
Boris Zbarsky — Back out bug 333177 "fix", since it would break searching across ­
}
(which comment had a wrong bug number)
fixed the warning,
without needing to modify line 961 ;->

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) class/interface test

[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.

Comment 14

[Boris Zbarsky [: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.

Comment 15

[[e-mail]]

Sorry, I'm not really the best reviewer for tests...

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 17

[Serge Gautherie (:sgautherie)]

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.

Comment 18

[Johnny Stenback (:jst)]

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

Comment 19

[Johnny Stenback (:jst)]

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.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) class/interface test [Checkin: Comment 20]

Bv1, with interface update(s), approved by jst over irc.

http://hg.mozilla.org/mozilla-central/rev/d863285d0093

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> I'll file a separate bug about the StripChars issue

I filed bug 456502.