[PATCH] D24997: [ClangTidy] Add UsingInserter and NamespaceAliaser

[Haojian Wu via cfe-commits]

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