mboehme marked an inline comment as done.
mboehme added inline comments.
================
Comment at: clang/lib/Sema/SemaType.cpp:170
/// Whether we saved the attributes in the decl spec.
bool hasSavedAttrs;
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > 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.)
> > I just tried:
> >
> > ```
> > // Don't try to save them multiple times.
> > if (!savedAttrs.empty()) return;
> > ```
> >
> > I didn't get any test failures.
> >
> > Of course, this isn't proof that this is _really_ OK, but it's an
> > indication.
> >
> > You know this code better than I do, so I would defer to you: How do you
> > think I should proceed? Should I eliminate `hasSavedAttrs` too?
> I think you should eliminate it -- I don't think the behavior here was
> intentional (based on what I can tell from the code).
OK -- done!
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
