aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaType.cpp:170
/// Whether we saved the attributes in the decl spec.
bool hasSavedAttrs;
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Isn't the same true for this variable? It seems like:
> >
> > `trivial` == `savedAttrs.empty()`
> > `hasSavedAttrs` == `!savedAttrs.empty()`
> That's what I also thought at first -- but the situation for `hasSavedAttrs`
> is a bit more complicated. It gets set whenever `saveDeclAttrs()` is called,
> even if it doesn't actually end up saving any attributes (i.e. if
> `spec.getAttributes()` is empty).
>
> `hasSavedAttrs` is then used to prevent any further calls to
> `saveDeclSpecAttrs()` from doing anything:
>
> ```
> // Don't try to save them multiple times.
> if (hasSavedAttrs) return;
> ```
>
> Conceivably, `spec` _might_ have had attributes added to it in the meantime
> -- not sure? It might be OK to just replace this logic with `if
> (!savedAttrs.empty()) return;` -- but I'm not familiar enough with how this
> code gets used and therefore decided it would be better not to change it. Can
> you give additional input on this?
I have the impression that is an oversight in the code rather than an
intentional behavior. I think it may be okay to replace the logic with
`!savedAttrs.empty()` as well; if you do that, do you get any test failures?
(If you do, then that's a sign something else might be going on.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123783/new/
https://reviews.llvm.org/D123783
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
