hokein added a comment. Looks almost fine, a few code-style comments.
> ASTUtils.h:11 > +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H > +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H > +#include "clang/AST/AST.h" A blank line between define guard and include. > ASTUtils.h:17 > +namespace utils { > +const FunctionDecl *getSurroundingFunction(ASTContext &Context, > + const Stmt &Statement); Could you add a few documentation describing this interface? > ASTUtils.h:21 > +} // namespace tidy > +} // namespace clang > +#endif A blank line below. > NamespaceAliaser.cpp:8 > +// > +//===----------------------------------------------------------------------===// > +#include "NamespaceAliaser.h" A blank line. > NamespaceAliaser.cpp:14 > +#include "clang/ASTMatchers/ASTMatchers.h" > +#include "clang/Lex/Lexer.h" > +namespace clang { A blank line below. > NamespaceAliaser.cpp:41 > + > + // TODO(bangert): Doesn't consider the order of declarations. > + // If we accidentially pick an alias defined later in the function, FIXME. > NamespaceAliaser.cpp:44 > + // the output won't compile. > + // TODO(bangert): Also doesn't consider file or class-scope aliases. > + FIXME. > NamespaceAliaser.cpp:85 > + StringRef Namespace) const { > + auto *Function = getSurroundingFunction(Context, Statement); > + auto FunctionAliases = AddedAliases.find(Function); I'd prefer to declare it `const auto *`. > NamespaceAliaser.h:8 > +// > +//===----------------------------------------------------------------------===// > +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NAMESPACEALIASER_H A blank line. > NamespaceAliaser.h:21 > +namespace tidy { > +namespace utils { > +// This class creates function-level namespace aliases. A blank line below. > NamespaceAliaser.h:28 > + // Statement. Picks the first available name from \p Abbreviations. > + // Returns ``llvm::None`` if an alias already exists or these is an error. > + llvm::Optional<FixItHint> s/these/there? > NamespaceAliaser.h:41 > + const SourceManager &SourceMgr; > + std::map<const FunctionDecl *, llvm::StringMap<std::string>> AddedAliases; > +}; Use `llvm::DenseMap` instead. > UsingInserter.cpp:49 > + > + // TODO(bangert): This declaration could be masked. Investigate if > + // there is a way to avoid using Sema. Use `FIXME` instead of `TODO(bangert).` > UsingInserter.h:23 > + > +// UsingInserter adds function-level using declarations. > +class UsingInserter { Could you elaborate a bit more? > UsingInserter.h:41 > + const SourceManager &SourceMgr; > + std::set<std::pair<const FunctionDecl *, std::string>> AddedUsing; > +}; It'd be clearer to use a typedef for `std::pair<const FunctionDecl *, std::string>` here. https://reviews.llvm.org/D24997 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits