aaron.ballman 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());
   }
----------------
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.


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

https://reviews.llvm.org/D121201

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to