[PATCH] D121201: [clang] Merge the SourceRange into ParsedAttributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:2516-2521
+  void takeAttributes(ParsedAttributes &attrs) {
     Attrs.takeAllFrom(attrs);
 
-    if (!lastLoc.isInvalid())
-      SetRangeEnd(lastLoc);
+    if (attrs.Range.getEnd().isValid())
+      SetRangeEnd(attrs.Range.getEnd());
   }
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > tbaeder wrote:
> > > > aaron.ballman wrote:
> > > > > 
> > > > I blindly changed this and it took me a while to figure out that's wrong from the test failures: 
> > > > 
> > > > `Attrs.takeAllFrom(Attrs)`...
> > > Oh god, I'm so sorry for that terrible suggestion, I hadn't spotted I was reusing the name. Feel free to go with `A` or `PA` or something for the parameter name to avoid that conflict.
> > Haha, no problem. Do you think adding an assertion for this case to `takeAllFrom()` (and `takeOneFrom()`) makes sense?
> An assertion that the attributes are actually taken from the argument (so validating the size of the container after taking from it)? Probably wouldn't hurt.
I'd suggest more of a 'Make sure other side is not this side' :D 


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:813
   void takeAllFrom(AttributePool &pool) {
     takePool(pool);
     pool.Attrs.clear();
----------------
so: `assert(&pool != this && "Stealing from yourself?  Super bad...")`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121201/new/

https://reviews.llvm.org/D121201