../../third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp:59:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas] |
||||
Issue description[ 22170/25096 - 300 processes @ 44.7/s : 496.229s ]CXX obj/third_party/WebKit/Source/modules/media_controls/media_controls/MediaControlsResourceLoader.o ../../third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp:59:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas] #pragma GCC diagnostic pop
,
Oct 5 2017
There's some discussion about this on https://chromium-review.googlesource.com/c/chromium/src/+/695328/7/third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp Ken wrote: """This is a warning which for some reason we do not treat as an error, but I think it definitely needs to be fixed. The issue is that there ought to be a corresponding push above the previous diagnostic pragma. i.e.: #pragma GCC diagnostic push #pragma GCC diagnostic warning "-Wunknown-pragmas" otherwise popping here makes no sense. """ Note that this only started happening after r506384 landed (which changed the "diagnostic ignored" to "diagnostic warning"). I don't know why. But Ken's suggestion fixes the warning (add "#pragma GCC diagnostic push" to line 40). beccahughes@ are you able to land this fix soon?
,
Oct 5 2017
I will point out that if this builds without the corresponding push, that means that you don't need the pop at all. Given that the warning you're trying to supress is for "unknown-pragmas", and I don't see any pragmas, this seems unnecessary. I'm guessing this was included when "#pragma warning(default : 4068)" was still compiled on non-WIN platforms.
,
Oct 5 2017
Yeah, I'm a bit confused about what this pragma is solving since there don't seem to be any pragmas in between. I guess a diagnostic pop without a push counts as a -Wunknown-pragmas warning, which is why it wasn't being flagged as an issue when it was set to ignore. If #3 is correct that this was only put there to work around the Microsoft-specific pragma on non-Windows platforms, then this shouldn't be necessary given that those are in an OS_WIN condition. BTW, here is GCC's manual page on the subject: https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html (I know we're using Clang, but I assume they implemented the same thing.)
,
Oct 5 2017
The pragmas only appear for internal builds when resource whitelisting is turned on. Grit injects a pragma into the resource header files which then throws an "unknown pragma" warning on compile time. Grit will then run generate_resource_whitelist.py which will grep over the build logs looking for those warnings. Any resource that does not have a warning is stripped from the final build. This then caused crashes since resources were missing from Chrome. We compile Blink with "-Werror" so these warning also cause build failures, hence the pragma to output them as warnings and not errors. The "pragma diagnostic pop" warning is a bug though and I will fix that.
,
Oct 5 2017
#5 makes sense. I'm not sure, though, why this isn't an error compiling Chromium, since I thought we compiled everything with -Werror. (?) Anyway, thanks for fixing the warning.
,
Oct 5 2017
CL to fix this is out for review: https://chromium-review.googlesource.com/c/chromium/src/+/702275
,
Oct 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8170e999fd677b429c2b75f7a212e3087ee81a9e commit 8170e999fd677b429c2b75f7a212e3087ee81a9e Author: Becca Hughes <beccahughes@chromium.org> Date: Thu Oct 05 11:37:58 2017 Fix compiler warning in Blink. Fix "no matching push" compiler warning in Blink. BUG= 771752 Change-Id: I7a4b49c99c2f74cf18c03c025994284730ce9536 Reviewed-on: https://chromium-review.googlesource.com/702275 Commit-Queue: Becca Hughes <beccahughes@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#506701} [modify] https://crrev.com/8170e999fd677b429c2b75f7a212e3087ee81a9e/third_party/WebKit/Source/modules/media_controls/MediaControlsResourceLoader.cpp
,
Oct 5 2017
,
Oct 5 2017
Issue 771968 has been merged into this issue. |
||||
►
Sign in to add a comment |
||||
Comment 1 by mgiuca@chromium.org
, Oct 5 2017