Summary: | -Wtype-limits warning spam from CCallHelpers.h | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | JavaScriptCore | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, darin, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=212006 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2020-05-10 13:09:36 PDT
I'm going to add IGNORE_WARNINGS macros, that's all I can really do here. Created attachment 399130 [details]
Patch
Can you try adding if constexpr checks around the for statements like so: if constexpr (!!NumberOfRegisters) { for (unsigned i = 0; i < NumberOfRegisters; ++i) { if (sources[i] != destinations[i]) pairs.append(std::make_pair(sources[i], destinations[i])); } } if constexpr (!!TargetSize) { for (unsigned i = 0; i < TargetSize; i++) { ASSERT(sourceArray[i] != static_cast<int32_t>(InfoTypeForReg<RegType>::InvalidIndex)); result[i] = sourceArray[i]; } } See if that helps. If it does, add a comment about why the if constexpr was added. Are any of the EWS using GCC10 yet? If not, are there plans to migrate that way? It's going to be difficult to keep these from continuing to manifest without the EWS. Comment on attachment 399130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399130&action=review > Source/JavaScriptCore/jit/CCallHelpers.h:103 > for (unsigned i = 0; i < NumberOfRegisters; ++i) { Is this warning just about when NumberOfRegisters is 0? (In reply to Mark Lam from comment #3) > Can you try adding if constexpr checks We should try this! Thanks for being responsive to my attempts to keep the GCC build nice and clean. :) (In reply to Mark Lam from comment #4) > Are any of the EWS using GCC10 yet? If not, are there plans to migrate that > way? It's going to be difficult to keep these from continuing to manifest > without the EWS. I'll let Igalia answer that. afaik the current EWS just uses whatever GCC is provided by Debian stable. Having more EWS would certainly be nice. Also note that EWS does not build with -Werror, so there's no way to notice warnings even on current compiler versions. I think changing that would catch probably 90% of GCC warnings before they're committed to trunk, since relatively few warnings are new warnings. (In reply to Darin Adler from comment #5) > > Source/JavaScriptCore/jit/CCallHelpers.h:103 > > for (unsigned i = 0; i < NumberOfRegisters; ++i) { > > Is this warning just about when NumberOfRegisters is 0? Right. (In reply to Mark Lam from comment #3) > Can you try adding if constexpr checks around the for statements like so: Oooh, didn't know about constexpr if. I think that worked, nice! Will let it continue building a while longer just to be certain. Created attachment 399179 [details]
Patch
Comment on attachment 399179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399179&action=review > Source/JavaScriptCore/jit/CCallHelpers.h:103 > + if constexpr (!!NumberOfRegisters) { I don’t think we need the !! > Source/JavaScriptCore/jit/CCallHelpers.h:320 > + if constexpr (!!TargetSize) { Ditto. Comment on attachment 399179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399179&action=review >> Source/JavaScriptCore/jit/CCallHelpers.h:103 >> + if constexpr (!!NumberOfRegisters) { > > I don’t think we need the !! I already tried without the !!. Clang complains about ints not matching bools. That's why I added it. How about if constexpr (NumberOfRegisters > 0)? That's entirely equivalent and complies with WebKit code style. The clearest thing to do would be to write '== 0', but our code style doesn't allow that. (In reply to Michael Catanzaro from comment #11) > How about if constexpr (NumberOfRegisters > 0)? That's entirely equivalent > and complies with WebKit code style. > > The clearest thing to do would be to write '== 0', but our code style > doesn't allow that. IMHO, (NumberOfRegisters > 0) is fine or you can just leave it as (!!NumberOfRegisters). Created attachment 399498 [details]
Patch for landing
Apparently the > 0 construction doesn't avoid the build warning...? Huh.... (In reply to Michael Catanzaro from comment #14) > Apparently the > 0 construction doesn't avoid the build warning...? Huh.... Ah no, it probably does, I just reverted my local changes out of habit before starting a test build to check it. :P Committed r261754: <https://trac.webkit.org/changeset/261754> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399498 [details]. |