Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490473007:
Improved check for trivial templates. (Closed)
DescriptionImproved check for trivial templates.
This is a follow-up to/fix for https://codereview.chromium.org/2483973003 (Treat
std::atomic_int as a trivial class member when checking for non-trivial
classes.).
BUG=663463
Committed: https://crrev.com/59b560a688614d2d283ff12f8b36e2fc29d92932
Cr-Commit-Position: refs/heads/master@{#431450}
Patch Set 1 #Patch Set 2 : atomic_int is not necessary called that at this point in the check. #
Total comments: 3
Patch Set 3 : Check in Type::Typedef, and add a test expectations file. #
Total comments: 1
Patch Set 4 : Rebase to re-add test! #Patch Set 5 : Check isStdNamespace, too. #
Total comments: 3
Patch Set 6 : Check in case Type::Typedef, and check isStdNamespace. #Patch Set 7 : Enable GNU asm for spin_lock. #Patch Set 8 : GNU asm change was out of scope for this CL. #
Messages
Total messages: 41 (12 generated)
palmer@chromium.org changed reviewers: + dcheng@chromium.org, erg@chromium.org
The previous CL wasn't quite the trick, alas.
https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:667: base_name == "__atomic_base") { I'm wondering if we should just intercept this in Type::Typedef: the standard defines atomic_int to be a typedef for atomic<int>, so it might be more robust than trying to match against implementation-specified names?
hans@chromium.org changed reviewers: + hans@chromium.org
https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:667: base_name == "__atomic_base") { On 2016/11/09 06:33:45, dcheng wrote: > I'm wondering if we should just intercept this in Type::Typedef: the standard > defines atomic_int to be a typedef for atomic<int>, so it might be more robust > than trying to match against implementation-specified names? (Actually, the standard says atomic_int is either a typedef to the specialization or to a base class of it.) But yeah, if we could check the name of the typedef that might be better.
https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/20001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:667: base_name == "__atomic_base") { > I'm wondering if we should just intercept this in Type::Typedef: the standard defines atomic_int to be a typedef for atomic<int>, so it might be more robust than trying to match against implementation-specified names? Done (and that resulted in a smaller diff, which is always nice).
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(I think the DeclContext should always be non-null... but maybe C++ is weirder than I think. I'm asking on IRC if this is possible) Also, this might need to be rebased on top of the earlier revert, since I think we'll need to recommit the test changes, right? https://codereview.chromium.org/2490473007/diff/40001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/40001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:679: if (name == "atomic_int") { Nit: I wonder if we should get the decl context and call isStdNamespace() on it?
> (I think the DeclContext should always be non-null... but maybe C++ is weirder than I think. I'm asking on IRC if this is possible) See what you think now; I check anyway. > Also, this might need to be rebased on top of the earlier revert, since I think we'll need to recommit the test changes, right? Done. > Nit: I wonder if we should get the decl context and call isStdNamespace() on it? Done.
> > Nit: I wonder if we should get the decl context and call isStdNamespace() on it? > > Done. Wait... why does my test still pass? That seems wrong.
On 2016/11/10 02:28:56, palmer wrote: > > > Nit: I wonder if we should get the decl context and call isStdNamespace() on > it? > > > > Done. > > Wait... why does my test still pass? That seems wrong. I'm not sure =) Unfortunately, update.py's default is to build without debugging symbols (and debugging the plugin is a bit of a pain anyway). If you're up for an adventure, muck with the cflags around https://cs.chromium.org/chromium/src/tools/clang/scripts/update.py?rcl=0&l=630 and add -g. Rebuild, then try running the command the test script uses inside gdb, and see if it's actually hitting the code you added. The easy non-adventurous way to do this is to sprinkle assert(false)/printf() throughout the code and try to debug the control flow that way =P
(FOrgot to publish the lone comment) https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:680: if (auto* context = decl->getDeclContext()) { I've learned that null checking this shouldn't be necessary (since it's not possible to have a TranslationUnitDecl here). hans@, how do you feel about not null-checking this?
https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:680: if (auto* context = decl->getDeclContext()) { On 2016/11/10 06:58:56, dcheng wrote: > I've learned that null checking this shouldn't be necessary (since it's not > possible to have a TranslationUnitDecl here). > > hans@, how do you feel about not null-checking this? I don't think it should be necessary. I don't see how a typedef could be declared not in a context.
https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/2490473007/diff/80001/tools/clang/plugins/Fin... tools/clang/plugins/FindBadConstructsConsumer.cpp:680: if (auto* context = decl->getDeclContext()) { > > hans@, how do you feel about not null-checking this? > > I don't think it should be necessary. I don't see how a typedef could be declared not in a context. Done.
> tools/clang/plugins/FindBadConstructsConsumer.cpp:667: base_name ==
"__atomic_base") {
> I'm wondering if we should just intercept this in Type::Typedef: the standard
defines atomic_int to be a typedef for atomic<int>, so it might be more robust
than trying to match against implementation-specified names?
Done.
LGTM This is verified to unblock the SpinLock changes, right?
lgtm
lgtm
> This is verified to unblock the SpinLock changes, right?
It gets me to the next exciting error, which is unrelated:
../../base/synchronization/spin_lock.cc:67:9: error: GNU-style inline assembly
is disabled
YIELD_PROCESSOR;
But the next patchset fixes that. Turns out you have to explicitly enable GNU
asm for Clang.
On 2016/11/11 01:15:48, palmer wrote: > But the next patchset fixes that. Turns out you have to explicitly enable GNU > asm for Clang. That change seems unrelated to the clang plugin patch.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org, erg@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2490473007/#ps120001 (title: "Enable GNU asm for spin_lock.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hans@chromium.org
not LGTM; the base/BUILD.gn change is out of scope
> not LGTM; the base/BUILD.gn change is out of scope dcheng asked if I could make it into this CL so that we don't have to roll the plugins multiple times. Would you rather have 2 CLs, or do you object to the actual change itself? (If so, what's a better way to go? Thanks for clues.)
> dcheng asked if I could make it into this CL so that we don't have to roll the plugins multiple times. Would you rather have 2 CLs, or do you object to the actual change itself? (If so, what's a better way to go? Thanks for clues.) Ugh, I am just typing too fast. Sorry. dcheng said that when he and I thought maybe it was a plugin causing the GNU asm build failure. I crossed my own streams. :) I'll roll up a 2nd CL. Sorry!
On 2016/11/11 01:22:27, palmer wrote: > > not LGTM; the base/BUILD.gn change is out of scope > > dcheng asked if I could make it into this CL so that we don't have to roll the > plugins multiple times. Would you rather have 2 CLs, or do you object to the > actual change itself? (If so, what's a better way to go? Thanks for clues.) I don't have an opinion about the change itself, but it should go in a separate CL.
On 2016/11/11 01:23:58, palmer wrote: > > dcheng asked if I could make it into this CL so that we don't have to roll the > plugins multiple times. I don't think that change is related to the plugin?
> I don't have an opinion about the change itself, but it should go in a separate CL. Done.
On 2016/11/11 01:25:18, palmer wrote: > > I don't have an opinion about the change itself, but it should go in a > separate CL. > > Done. lgtm again :-)
On 2016/11/11 01:25:57, hans wrote: > On 2016/11/11 01:25:18, palmer wrote: > > > I don't have an opinion about the change itself, but it should go in a > > separate CL. > > > > Done. > > lgtm again :-) lgtm Sorry for not being clearer on chat; I just wanted to make sure there were no other issues in the clang plugin itself, since Blink hasn't historically enforced many of these checks. It seems appropriate to split other non-plugin fixes out.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org Link to the patchset: https://codereview.chromium.org/2490473007/#ps140001 (title: "GNU asm change was out of scope for this CL.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Improved check for trivial templates. This is a follow-up to/fix for https://codereview.chromium.org/2483973003 (Treat std::atomic_int as a trivial class member when checking for non-trivial classes.). BUG=663463 ========== to ========== Improved check for trivial templates. This is a follow-up to/fix for https://codereview.chromium.org/2483973003 (Treat std::atomic_int as a trivial class member when checking for non-trivial classes.). BUG=663463 Committed: https://crrev.com/59b560a688614d2d283ff12f8b36e2fc29d92932 Cr-Commit-Position: refs/heads/master@{#431450} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/59b560a688614d2d283ff12f8b36e2fc29d92932 Cr-Commit-Position: refs/heads/master@{#431450} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
