aaron.ballman added a comment. In D93630#2480070 <https://reviews.llvm.org/D93630#2480070>, @vsavchenko wrote:
> In D93630#2479757 <https://reviews.llvm.org/D93630#2479757>, @aaron.ballman > wrote: > >> There are attributes like `nomerge` which are both a declaration and a >> statement attribute, and there are other attributes like `suppress` that are >> statement attributes which can reasonably be written on a declaration >> statement. So the scenario I am worried about is when the code is being >> parsed as a declaration, the user adds one of these attributes and the >> decision changes to parse as a statement rather than a declaration. I'm not >> yet convinced we can't run into that situation with this change, but the >> implicit `int` thing was the only major edge case I could come up with and >> I've alllllmost convinced myself we can't run into an issue with it. I think >> our parser predicate test (on line 218 of your patch) for whether something >> is a declaration statement or not is what's helping avoid most of the >> parsing concerns I have because they usually will find some declaration >> specifier to help get the right answer. > > I actually thought about this. Essentially there are 3 interesting cases > here: > > 1. the user has a declaration that is parsed as a declaration only because of > the attribute and wants to add a statement attribute > > __attribute__((X)) i = 12; > // into > __attribute__((X, fallthrough)) i = 12; > > We don't want to break working code, so we maintain the rule and parse it as > a declaration. > > 2. the user has a statement and wants to add a statement attribute > > i = 12; // assignment > // into > __attribute__((stmt_attr)) i = 12; // still assignment > > We don't use the rule and don't break the code, that's why we do it! > > 3. the user has a declaration-or-statement attribute on a declaration that is > parsed as declaration only because of this attribute > > __attribute__((both-decl-and-stmt)) i = 12; // implicit int declaration > // after the Clang update > __attribute__((both-decl-and-stmt)) i = 12; // error! > > That is a regression, but it looks like statement attributes are pretty rare, > let alone declaration-or-statement attributes in C. Also, it might cause the > problem only when the user relied on the "attribute -> declaration" rule. > So, it seems pretty low chance, and we get a bit more consistency in what > "statement attribute" means. I agree with these three cases. I did come up with a fourth case: the user wants to attribute something that is a statement which declares things but is not a declaration statement. e.g., `__attribute__((whatever)) [](){}();` (or similar using blocks instead of lambdas). We don't parse this construct on trunk (https://godbolt.org/z/3e83of) and I tried it with your patch applied and it still wouldn't parse, so it may make for a good test case to add. >> FWIW, while testing various situations out, I think I found a Clang bug >> where we reject valid code (https://godbolt.org/z/ooqMvP) that *might* cause >> us problems if we apply this patch and ever fix that rejects valid bug. We >> should not be rejecting any of these functions, but we fail to note the >> implicit `int` with the declaration within `foo()` (note how the presence of >> the `extern` declaration specifier in `quux()` fixes things). > > Yikes, that's pretty bad! I filed https://bugs.llvm.org/show_bug.cgi?id=48672 for it. >>>> or `__attribute__((suppress)) g() { return 12; }` which may be intended to >>>> suppress the "type specifier missing" diagnostic (somewhat amusingly, >>>> since the user could also just add the `int` to the declaration since >>>> they're changing the code anyway). >>> >>> Again, we **do not force** the parser to parse whatever follows the >>> statement attribute as a statement. `__attribute__((suppress))` will not >>> change how we parse things here. >> >> I agree that it shouldn't (in theory), but I've not been able to convince >> myself that it doesn't (in practice). I'm getting there though, and I >> appreciate your patience while we discuss the concerns. > > I actually want to thank you instead. It is a pretty tricky situation here > with all this, so thank you for going so patiently from one case to another! Huttah for quality code review practices! :-) >> Concretely, I think we could use some more C test coverage for the various >> places where implicit `int` can appear in a declaration. Here are the places >> I could come up with off the top of my head: https://godbolt.org/z/6h3MTr > > That's awesome, I'll incorporate that into the patch! I'm going to see if I can run the patch against our internal corpus here at work to see if it shakes out any breakages to real world code. I've not tried this before, so I have no idea how long it will take before I get results back, but I'll let you know when I have them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93630/new/ https://reviews.llvm.org/D93630 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
