[PATCH] D13759: [Clang] Fix Clang-tidy modernize-use-auto warnings in headers and generated files; other minor cleanups.

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 11 07:51:19 PST 2015


aaron.ballman added inline comments.

================
Comment at: include/clang/AST/ASTVector.h:385
@@ -383,3 +384,3 @@
   // Allocate the memory from the ASTContext.
-  T *NewElts = new (C, llvm::alignOf<T>()) T[NewCapacity];
+  auto *NewElts = new (C, llvm::alignOf<T>()) T[NewCapacity];
 
----------------
Eugene.Zelenko wrote:
> hans wrote:
> > I'm not sure this one is an improvement.
> At least handling of new will be consistent. This will also silent Clang-tidy warning.
I'm not too concerned about the clang-tidy warning. It suggests that perhaps this checker could use an option to control this behavior, however.

I agree with Hans that a lot of the treatments of new expressions don't really seem to be improvements.

================
Comment at: include/clang/AST/DeclTemplate.h:1726
@@ -1725,4 +1725,3 @@
            "Already set to a class template partial specialization!");
-    SpecializedPartialSpecialization *PS
-      = new (getASTContext()) SpecializedPartialSpecialization();
+    auto *PS = new (getASTContext()) SpecializedPartialSpecialization();
     PS->PartialSpecialization = PartialSpec;
----------------
hans wrote:
> But here it does make it nicer :-)
I wonder if the heuristic is: when the new expression spans multiple lines, only replace the declaration type with auto if it causes the new expression to become a single-line expression. 

================
Comment at: include/clang/Analysis/Analyses/Consumed.h:266
@@ -266,2 +265,3 @@
   };
-}} // end namespace clang::consumed
+} // end namespace consumed
+} // end namespace clang
----------------
Eugene.Zelenko wrote:
> aaron.ballman wrote:
> > I don't know whether this is an improvement or not.
> This was changed for consistency with other code. I also think that it's good idea to place one statement per line.
This pattern is used in many places in the code base, it reduces vertical clutter, and it could be argued that it is easier to read because you don't have to interpret the namespace hierarchy.

================
Comment at: include/clang/Rewrite/Core/Rewriter.h:171
@@ -170,4 +170,3 @@
   const RewriteBuffer *getRewriteBufferFor(FileID FID) const {
-    std::map<FileID, RewriteBuffer>::const_iterator I =
-      RewriteBuffers.find(FID);
+    const auto I = RewriteBuffers.find(FID);
     return I == RewriteBuffers.end() ? nullptr : &I->second;
----------------
Eugene.Zelenko wrote:
> aaron.ballman wrote:
> > Not that this matters in the cases in this code, but const iterator is not the same as const_iterator. I think these changes are a step forward in terms of readability, but a step backwards in terms of the type system.
> I'm still learning C++11, so advices how to handle such situations properly are welcome!
In this case, I suppose it doesn't matter at all -- RewriteBuffers is a member variable being accessed from a const method, and so it will return a const_iterator. So technically, you don't even need the const here, just auto I.

The issue is more with code like:
```
struct S {
  SomeContainer C;

  void f() {
    SomeContainer::const_iterator I = C.begin();
  }
};
```
Replacing with auto here will deduce SomeContainer::iterator. The only way around that is with ugly casting, at which point I think leaving the declaration alone is the better choice.

For this code, I would remove the const and just leave it as auto I.


Repository:
  rL LLVM

http://reviews.llvm.org/D13759





More information about the cfe-commits mailing list