[PATCH] D33623: Make the parser close parens for you on EOF

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added a comment.

Do we want this change in the parser itself? or in clang-query?
clang-query could be helpful enough to add parens when it detect an 
`ET_ParserNoCloseParen` error, without changing the language here.


https://reviews.llvm.org/D33623



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

[PATCH] D33531: Clang-tidy readability: avoid const value return

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:31
+  isAnyPointer(),
+  references(type()),
+  templateTypeParmType(),

References are never const qualified, right?



Comment at: test/clang-tidy/readability-const-value-return.cpp:51
+const T f_returns_template_param();
+
+template 

We are missing one like:

template  T f();

where `T` is `const X`.

We also need to test for macros (and skip fixes there)


Repository:
  rL LLVM

https://reviews.llvm.org/D33531



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

[PATCH] D41779: [clang-tidy] Fix DanglingHandleCheck for the correct conversion operation between basic_string and basic_string_view.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: hokein.
Herald added subscribers: cfe-commits, xazax.hun, klimek.

Fix DanglingHandleCheck to handle the final implementation of std::string and 
std::string_view.
These use a conversion operator instead of a conversion constructor.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41779

Files:
  clang-tidy/bugprone/DanglingHandleCheck.cpp
  test/clang-tidy/bugprone-dangling-handle.cpp


Index: test/clang-tidy/bugprone-dangling-handle.cpp
===
--- test/clang-tidy/bugprone-dangling-handle.cpp
+++ test/clang-tidy/bugprone-dangling-handle.cpp
@@ -45,19 +45,23 @@
   value_type& operator[](Key&& key);
 };
 
+class basic_string_view;
+
 class basic_string {
  public:
   basic_string();
   basic_string(const char*);
+
+  operator basic_string_view() const noexcept;
+
   ~basic_string();
 };
 
 typedef basic_string string;
 
 class basic_string_view {
  public:
   basic_string_view(const char*);
-  basic_string_view(const basic_string&);
 };
 
 typedef basic_string_view string_view;
Index: clang-tidy/bugprone/DanglingHandleCheck.cpp
===
--- clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -25,8 +25,12 @@
 ast_matchers::internal::BindableMatcher
 handleFrom(const ast_matchers::internal::Matcher &IsAHandle,
const ast_matchers::internal::Matcher &Arg) {
-  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
-  hasArgument(0, Arg));
+  return expr(
+  anyOf(cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+ hasArgument(0, Arg)),
+cxxMemberCallExpr(hasType(cxxRecordDecl(IsAHandle)),
+  callee(memberExpr(member(cxxConversionDecl(,
+  on(Arg;
 }
 
 ast_matchers::internal::Matcher handleFromTemporaryValue(


Index: test/clang-tidy/bugprone-dangling-handle.cpp
===
--- test/clang-tidy/bugprone-dangling-handle.cpp
+++ test/clang-tidy/bugprone-dangling-handle.cpp
@@ -45,19 +45,23 @@
   value_type& operator[](Key&& key);
 };
 
+class basic_string_view;
+
 class basic_string {
  public:
   basic_string();
   basic_string(const char*);
+
+  operator basic_string_view() const noexcept;
+
   ~basic_string();
 };
 
 typedef basic_string string;
 
 class basic_string_view {
  public:
   basic_string_view(const char*);
-  basic_string_view(const basic_string&);
 };
 
 typedef basic_string_view string_view;
Index: clang-tidy/bugprone/DanglingHandleCheck.cpp
===
--- clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -25,8 +25,12 @@
 ast_matchers::internal::BindableMatcher
 handleFrom(const ast_matchers::internal::Matcher &IsAHandle,
const ast_matchers::internal::Matcher &Arg) {
-  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
-  hasArgument(0, Arg));
+  return expr(
+  anyOf(cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+ hasArgument(0, Arg)),
+cxxMemberCallExpr(hasType(cxxRecordDecl(IsAHandle)),
+  callee(memberExpr(member(cxxConversionDecl(,
+  on(Arg;
 }
 
 ast_matchers::internal::Matcher handleFromTemporaryValue(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41779: [clang-tidy] Fix DanglingHandleCheck for the correct conversion operation between basic_string and basic_string_view.

[Samuel Benzaquen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL322002: [clang-tidy] Fix DanglingHandleCheck for the correct 
conversion operation… (authored by sbenza, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D41779

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp


Index: clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -25,8 +25,12 @@
 ast_matchers::internal::BindableMatcher
 handleFrom(const ast_matchers::internal::Matcher &IsAHandle,
const ast_matchers::internal::Matcher &Arg) {
-  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
-  hasArgument(0, Arg));
+  return expr(
+  anyOf(cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+ hasArgument(0, Arg)),
+cxxMemberCallExpr(hasType(cxxRecordDecl(IsAHandle)),
+  callee(memberExpr(member(cxxConversionDecl(,
+  on(Arg;
 }
 
 ast_matchers::internal::Matcher handleFromTemporaryValue(
Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
@@ -45,19 +45,23 @@
   value_type& operator[](Key&& key);
 };
 
+class basic_string_view;
+
 class basic_string {
  public:
   basic_string();
   basic_string(const char*);
+
+  operator basic_string_view() const noexcept;
+
   ~basic_string();
 };
 
 typedef basic_string string;
 
 class basic_string_view {
  public:
   basic_string_view(const char*);
-  basic_string_view(const basic_string&);
 };
 
 typedef basic_string_view string_view;


Index: clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
@@ -25,8 +25,12 @@
 ast_matchers::internal::BindableMatcher
 handleFrom(const ast_matchers::internal::Matcher &IsAHandle,
const ast_matchers::internal::Matcher &Arg) {
-  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
-  hasArgument(0, Arg));
+  return expr(
+  anyOf(cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+ hasArgument(0, Arg)),
+cxxMemberCallExpr(hasType(cxxRecordDecl(IsAHandle)),
+  callee(memberExpr(member(cxxConversionDecl(,
+  on(Arg;
 }
 
 ast_matchers::internal::Matcher handleFromTemporaryValue(
Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
@@ -45,19 +45,23 @@
   value_type& operator[](Key&& key);
 };
 
+class basic_string_view;
+
 class basic_string {
  public:
   basic_string();
   basic_string(const char*);
+
+  operator basic_string_view() const noexcept;
+
   ~basic_string();
 };
 
 typedef basic_string string;
 
 class basic_string_view {
  public:
   basic_string_view(const char*);
-  basic_string_view(const basic_string&);
 };
 
 typedef basic_string_view string_view;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D18914: [clang-tidy] new readability-redundant-inline

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added a comment.

In D18914#396149 , @mgehre wrote:

> ...
>  I personally think that 1) should be used, because late one could move the 
> function definition to a source file (removing the inline) without having to 
> touch
>  the class declaration. I can extend this patch to transform 2) and 3) into 
> 1).
>
> Alternatively, I could add an option to choose between 1), 2) or 3).
>  What do you think?


I agree that (1) is preferred as it makes `inline` an implementation detail and 
doesn't pollute the class, but that is a style choice.
What we could do is transform (3) to (1). That is, if you provide _both_ 
`inline`s we remove one as it is redundant. That matches with the purpose of 
this check.

But changing (2) to (1) is not removing anything redundant, it is a style 
change.


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

https://reviews.llvm.org/D18914



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

[PATCH] D54307: [clang] overload ignoringParens for Expr

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:814
 /// would match the declaration for fp.
-AST_MATCHER_P(QualType, ignoringParens,
-  internal::Matcher, InnerMatcher) {
+AST_MATCHER_P_OVERLOAD(QualType, ignoringParens, internal::Matcher,
+   InnerMatcher, 0) {

JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > aaron.ballman wrote:
> > > > Can you do this via `AST_POLYMORPHIC_MATCHER_P` instead, given that the 
> > > > implementation is the same?
> > > Do you want me to add more types? e.g. `TypeLoc` has `IgnoreParens()`, 
> > > too. 
> > I'd not be opposed, given that we already expose the `typeLoc()` matcher. 
> > I'll leave that to your discretion.
> as discussed on IRC making it an `AST_POLYMORPHIC_MATCHER_P` does not seem to 
> work, as the polymorphism is only in the return type. We do need the `Node` 
> itself to be polymorphic (same type as returntype). The only working version 
> I got was using the overloads.
You can't use AST_POLYMORPHIC_MATCHER_P to overload on input types.
You could try using templates, but that will make registering the matcher 
harder.
Another one that does input+output polymorphism, `id`, is simply not supported 
dynamically.


Repository:
  rC Clang

https://reviews.llvm.org/D54307



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

[PATCH] D54407: Record the matcher type when storing a binding

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added a comment.

In https://reviews.llvm.org/D54407#1294934, @aaron.ballman wrote:

> Adding @sbenza in case he has opinions on this approach. I think it's 
> reasonable, but I also know that changes to the the AST matcher internals 
> sometimes have unintended side effects with the various instantiations.


The problem used to come with the number of template instantiations, but afaik 
this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

I also don't see any actual use of this newly stored data so I don't see the 
goal. Is this for diagnostics on failures or something like that?


Repository:
  rC Clang

https://reviews.llvm.org/D54407



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

[PATCH] D54404: Exclude matchers which can have multiple results

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added a comment.

In https://reviews.llvm.org/D54404#1296224, @aaron.ballman wrote:

> In https://reviews.llvm.org/D54404#1295426, @steveire wrote:
>
> > I acknowledge and share the future-proofing concern.
> >
> > We could possibly use something trait-based instead and put the trait 
> > beside the matcher definition in ASTMatchers.h, but that doesn't really 
> > solve the problem. It only moves the problem.
>
>
> If we could somehow incorporate it into the matcher definition itself, 
> though, it means we don't have two lists of things to keep updated. You're 
> right that it doesn't eliminate the problem -- we still have to know to ask 
> the question when reviewing new matchers (so perhaps something that requires 
> you to explicitly opt-in/out would be better).
>
> Adding @sbenza in case he has ideas.


We could put something in `MatcherDescriptor` to indicate this. However, I am 
still not sure what property are we looking for here.




Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606
+  // be used with particular arguments.
+  static std::vector ExcludedMatchers{
+  "allOf",

I don't think we are allowed to have non-trivial static storage duration 
objects like this.
You have to use llvm::ManagedStatic. Or just make it a raw array.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624
+  "hasAnyDeclaration",
+  "hasAnyName",
+  "hasAnyParameter",

I'm not sure what goes in this list.
`hasAnyName` is here but not `hasName`.
What is ambiguous about `hasAnyName`?


Repository:
  rC Clang

https://reviews.llvm.org/D54404



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

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:45
+  /// Evaluates this part to a string and appends it to `result`.
+  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+   std::string *Result) const = 0;

Why not use `llvm::Expected` as the return type?



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:75
+  // Copy constructor/assignment produce a deep copy.
+  StencilPart(const StencilPart &P) : Impl(P.Impl->clone()) {}
+  StencilPart(StencilPart &&) = default;

The interface API is all const. Why not keep a `shared_ptr` instead?
That way we don't have to have users implement a clone function.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:94
+  return false;
+return Impl->isEqual(*(Other.Impl));
+  }

Parens around Other.Impl are not necessary.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:110
+  template  static Stencil cat(Ts &&... Parts) {
+Stencil Stencil;
+Stencil.Parts.reserve(sizeof...(Parts));

I don't like naming the variable the same as the type.
You could just use `S` as the variable name. That is ok with llvm style for 
small functions.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:127
+  // Allow Stencils to operate as std::function, for compatibility with
+  // Transformer's TextGenerator. Calls `eval()` and asserts on failure.
+  std::string operator()(const ast_matchers::MatchFinder::MatchResult &) const;

"asserts" as it only fails in debug mode?



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:130
+
+  bool operator==(const Stencil &Other) const {
+return Parts == Other.Parts;

I usually prefer free functions for binary operators (friends is fine).
It makes them symmetric.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:155
+/// statement.
+StencilPart snode(llvm::StringRef Id);
+

`sNode` ?
ie camelCase



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:45
+
+namespace {
+// An arbitrary fragment of code within a stencil.

maybe this anon namespace should cover the functions above?



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:76
+
+static bool operator==(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;

If you define ==, imo you should define != too.
Otherwise just call it `isEqual`



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:94
+Error evalData(const T &Data, const MatchFinder::MatchResult &Match,
+   std::string *Result);
+

alignment.
(below too)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371



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

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:45
+  /// Evaluates this part to a string and appends it to `result`.
+  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+   std::string *Result) const = 0;

ymandel wrote:
> sbenza wrote:
> > Why not use `llvm::Expected` as the return type?
> so that each stencil part can append to a single string that we construct 
> when evaluating the whole stencil.  this was an (attempted?) optimization. if 
> concatenating is about as efficient, I'd prefer that approach. What do you 
> advise?
I think its fine as it is then.
I don't really think the performance would matter that much, but I don't see a 
reason to add all those copies unnecessarily.
Mention that `Result` is in an unspecified state in case of error.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:127
+  // Allow Stencils to operate as std::function, for compatibility with
+  // Transformer's TextGenerator. Calls `eval()` and asserts on failure.
+  std::string operator()(const ast_matchers::MatchFinder::MatchResult &) const;

ymandel wrote:
> sbenza wrote:
> > "asserts" as it only fails in debug mode?
> I thought `assert()` also fails in normal mode, based on my reading of the 
> llvm docs, but it's not explicit about this: 
> http://llvm.org/docs/ProgrammersManual.html#programmatic-errors
> 
> FWIW, I'm on the fence whether `eval()` should actually have the same 
> signature and similarly just crash the program on errors. Its not clear 
> there's any value to propogating the errors up the stack via `Expected`.
`assert` follows `NDEBUG`, which is not explicitly related to `-Ox`, but they 
usually go together.

Don't make it crash, please.
Otherwise it would be hard to have a refactoring service or alike.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:101
+S.Parts.reserve(sizeof...(Parts));
+auto Unused = {(S.push(std::forward(Parts)), true)...};
+(void)Unused;

We could simplify this code if you change `void push(T)` to instead 
`StencilPart wrap(T)`.
Then this could be:


```
Stencil S;
S.Parts = {wrap(std::forward(Parts))...};
return S;
```



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78
+
+template <> bool isEqualData(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;

Any particular reason to use function template specialization instead of 
function overloading?
The former is rare and thus harder to reason about.

(same for `evalData` below)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371



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

[PATCH] D86751: Add new warning for compound punctuation tokens that are split across macro expansions or split by whitespace.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza accepted this revision.
sbenza added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/Parser/compound-token-split.cpp:30
+
+[ // expected-warning-re ^}}'[' tokens introducing attribute appear in 
different source files}}
+#define LSQUARE

Does this count as a "different" source file?
Or is it just an implementation detail of how clang uses FileIDs ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86751

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

[PATCH] D110586: Update `DynTypedNode` to support the conversion of `TypeLoc`s.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang/include/clang/AST/ASTTypeTraits.h:472
+  assert(ASTNodeKind::getFromNodeKind().isBaseOf(NodeKind));
+  return *static_cast(reinterpret_cast(Storage));
+}

The create/get don't seem to match.
We are constructing a `BaseT` object in `create()`, so this `static_cast` to a 
derived type is UB?
That memory does not contain a `T`.

If we instead construct a `T` in `create()`, we should be able to read it as a 
`BaseT*` (assuming `is_pointer_interconvertible_base_of` is true)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110586

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

[PATCH] D110586: Update `DynTypedNode` to support the conversion of `TypeLoc`s.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang/unittests/AST/ASTTypeTraitsTest.cpp:212
+  DynTypedNode Node = DynTypedNode::create(tl);
+  EXPECT_TRUE(Node == Node);
+  EXPECT_FALSE(Node < Node);

I don't know what we are trying to check with this self equivalence.
Did you mean `Node == tl`?



Comment at: clang/unittests/AST/ASTTypeTraitsTest.cpp:225
+  const auto ptl =
+  matches[0].getNodeAs("ptl")->castAs();
+  DynTypedNode Node = DynTypedNode::create(ptl);

We should check that we can do getNodeAs and getNodeAs.
That is the goal of having the hierarchy in DynTypedNode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110586

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

[PATCH] D110586: Update `DynTypedNode` to support the conversion of `TypeLoc`s.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza accepted this revision.
sbenza added inline comments.



Comment at: clang/unittests/AST/ASTTypeTraitsTest.cpp:234
+  EXPECT_TRUE(PointerTypeLocNode == PointerTypeLocNode);
+  EXPECT_FALSE(PointerTypeLocNode < PointerTypeLocNode);
+}

Maybe also check

```
EXPECT_EQ(&tl, &ptl);
```

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110586

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

[PATCH] D29622: Add a batch query and replace tool based on AST matchers.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang-query/QueryReplace.h:35-36
+
+  /// \brief Replacement text. %"identifier" will be substituted by the text of
+  /// an identifier.
+  std::string ToTemplate;

klimek wrote:
> This doesn't seem to come up in the test? (and I don't understand what it's 
> trying to tell me :)
In the other change the identifier was specified with ${identifier}, not 
%"identifier".
They should be consistent, no?


https://reviews.llvm.org/D29622



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.emplace_back(Callback);
+  }

Why emplace_back instead of push_back?



Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+  for (size_t Index = 0; Index < ToTemplate.size();) {
+if (ToTemplate[Index] == '$') {
+  if (ToTemplate.substr(Index, 2) == "$$") {

We should do the parsing in the constructor, instead of on each match.
We could store a vector of {string, bool is_id;} that we just iterate during 
each match.
It also has the advantage of hitting the error+assert before in the run.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:174
+ToTemplate.substr(Index + 2, EndOfIdentifier - Index - 2);
+if (NodeMap.count(SourceNodeName) == 0) {
+  llvm::errs()

This is making two lookups into the map when one is enough.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:192
+  size_t NextIndex = ToTemplate.find('$', Index + 1);
+  ToText = ToText + ToTemplate.substr(Index, NextIndex - Index);
+  Index = NextIndex;

X += instead of X = X +


https://reviews.llvm.org/D29621



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

[PATCH] D41998: [clang-tidy] Expand readability-redundant-smartptr-get to understand implicit converions to bool in more contexts.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: hokein.
Herald added subscribers: cfe-commits, xazax.hun, klimek.

Expand readability-redundant-smartptr-get to understand implicit converions to 
bool in more contexts.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41998

Files:
  clang-tidy/readability/RedundantSmartptrGetCheck.cpp
  test/clang-tidy/readability-redundant-smartptr-get.cpp

Index: test/clang-tidy/readability-redundant-smartptr-get.cpp
===
--- test/clang-tidy/readability-redundant-smartptr-get.cpp
+++ test/clang-tidy/readability-redundant-smartptr-get.cpp
@@ -9,13 +9,15 @@
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 template 
 struct shared_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 }  // namespace std
@@ -28,6 +30,7 @@
   Bar* operator->();
   Bar& operator*();
   Bar* get();
+  explicit operator bool() const;
 };
 struct int_ptr {
   int* get();
@@ -110,6 +113,23 @@
   // CHECK-MESSAGES: uu.get() == nullptr;
   // CHECK-FIXES: bool bb = uu == nullptr;
 
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+  bb = u.get() ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = u.get() ? true : false;
+  // CHECK-FIXES: bb = u ? true : false;
+
   bb = nullptr != ss->get();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
@@ -146,10 +166,6 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
-  if (x.get());
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
-  // CHECK-MESSAGES: if (x.get());
-  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
@@ -175,4 +191,6 @@
 
   int_ptr ip;
   bool bb = ip.get() == nullptr;
+  bb = !ip.get();
+  bb = ip.get() ? true : false;
 }
Index: clang-tidy/readability/RedundantSmartptrGetCheck.cpp
===
--- clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -18,6 +18,7 @@
 namespace readability {
 
 namespace {
+
 internal::Matcher callToGet(const internal::Matcher &OnClass) {
   return cxxMemberCallExpr(
  on(expr(anyOf(hasType(OnClass),
@@ -51,6 +52,20 @@
   unaryOperator(hasOperatorName("*"),
 hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
   Callback);
+
+  // Catch '!ptr.get()'
+  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
+  QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()));
+  Finder->addMatcher(
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
+  Callback);
+
+  // Catch 'if(ptr.get())'
+  Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
+
+  // Catch 'ptr.get() ? X : Y'
+  Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
+ Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
@@ -72,11 +87,6 @@
  hasEitherOperand(callToGet(IsAKnownSmartptr))),
   Callback);
 
-  // Matches against if(ptr.get())
-  Finder->addMatcher(
-  ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr,
-  Callback);
-
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41998: [clang-tidy] Expand readability-redundant-smartptr-get to understand implicit converions to bool in more contexts.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza updated this revision to Diff 129880.
sbenza marked an inline comment as done.
sbenza added a comment.

minor fix


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41998

Files:
  clang-tidy/readability/RedundantSmartptrGetCheck.cpp
  test/clang-tidy/readability-redundant-smartptr-get.cpp

Index: test/clang-tidy/readability-redundant-smartptr-get.cpp
===
--- test/clang-tidy/readability-redundant-smartptr-get.cpp
+++ test/clang-tidy/readability-redundant-smartptr-get.cpp
@@ -9,13 +9,15 @@
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 template 
 struct shared_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 }  // namespace std
@@ -28,6 +30,7 @@
   Bar* operator->();
   Bar& operator*();
   Bar* get();
+  explicit operator bool() const;
 };
 struct int_ptr {
   int* get();
@@ -110,6 +113,23 @@
   // CHECK-MESSAGES: uu.get() == nullptr;
   // CHECK-FIXES: bool bb = uu == nullptr;
 
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+  bb = u.get() ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = u.get() ? true : false;
+  // CHECK-FIXES: bb = u ? true : false;
+
   bb = nullptr != ss->get();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
@@ -146,10 +166,6 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
-  if (x.get());
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
-  // CHECK-MESSAGES: if (x.get());
-  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
@@ -175,4 +191,6 @@
 
   int_ptr ip;
   bool bb = ip.get() == nullptr;
+  bb = !ip.get();
+  bb = ip.get() ? true : false;
 }
Index: clang-tidy/readability/RedundantSmartptrGetCheck.cpp
===
--- clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -51,6 +51,20 @@
   unaryOperator(hasOperatorName("*"),
 hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
   Callback);
+
+  // Catch '!ptr.get()'
+  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
+  QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()));
+  Finder->addMatcher(
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
+  Callback);
+
+  // Catch 'if(ptr.get())'
+  Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
+
+  // Catch 'ptr.get() ? X : Y'
+  Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
+ Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
@@ -72,11 +86,6 @@
  hasEitherOperand(callToGet(IsAKnownSmartptr))),
   Callback);
 
-  // Matches against if(ptr.get())
-  Finder->addMatcher(
-  ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr,
-  Callback);
-
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41998: [clang-tidy] Expand readability-redundant-smartptr-get to understand implicit converions to bool in more contexts.

[Samuel Benzaquen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL322497: [clang-tidy] Expand 
readability-redundant-smartptr-get to understand implicit… (authored by sbenza, 
committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D41998

Files:
  clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp

Index: clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -51,6 +51,20 @@
   unaryOperator(hasOperatorName("*"),
 hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
   Callback);
+
+  // Catch '!ptr.get()'
+  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
+  QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()));
+  Finder->addMatcher(
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
+  Callback);
+
+  // Catch 'if(ptr.get())'
+  Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
+
+  // Catch 'ptr.get() ? X : Y'
+  Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
+ Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
@@ -72,11 +86,6 @@
  hasEitherOperand(callToGet(IsAKnownSmartptr))),
   Callback);
 
-  // Matches against if(ptr.get())
-  Finder->addMatcher(
-  ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr,
-  Callback);
-
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
Index: clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp
@@ -9,13 +9,15 @@
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 template 
 struct shared_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 }  // namespace std
@@ -28,6 +30,7 @@
   Bar* operator->();
   Bar& operator*();
   Bar* get();
+  explicit operator bool() const;
 };
 struct int_ptr {
   int* get();
@@ -110,6 +113,23 @@
   // CHECK-MESSAGES: uu.get() == nullptr;
   // CHECK-FIXES: bool bb = uu == nullptr;
 
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+  bb = u.get() ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = u.get() ? true : false;
+  // CHECK-FIXES: bb = u ? true : false;
+
   bb = nullptr != ss->get();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
@@ -146,10 +166,6 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
-  if (x.get());
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
-  // CHECK-MESSAGES: if (x.get());
-  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
@@ -175,4 +191,6 @@
 
   int_ptr ip;
   bool bb = ip.get() == nullptr;
+  bb = !ip.get();
+  bb = ip.get() ? true : false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41998: [clang-tidy] Expand readability-redundant-smartptr-get to understand implicit converions to bool in more contexts.

[Samuel Benzaquen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE322497: [clang-tidy] Expand 
readability-redundant-smartptr-get to understand implicit… (authored by sbenza, 
committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41998?vs=129880&id=129882#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41998

Files:
  clang-tidy/readability/RedundantSmartptrGetCheck.cpp
  test/clang-tidy/readability-redundant-smartptr-get.cpp

Index: clang-tidy/readability/RedundantSmartptrGetCheck.cpp
===
--- clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -51,6 +51,20 @@
   unaryOperator(hasOperatorName("*"),
 hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
   Callback);
+
+  // Catch '!ptr.get()'
+  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
+  QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()));
+  Finder->addMatcher(
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
+  Callback);
+
+  // Catch 'if(ptr.get())'
+  Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
+
+  // Catch 'ptr.get() ? X : Y'
+  Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
+ Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
@@ -72,11 +86,6 @@
  hasEitherOperand(callToGet(IsAKnownSmartptr))),
   Callback);
 
-  // Matches against if(ptr.get())
-  Finder->addMatcher(
-  ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr,
-  Callback);
-
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
Index: test/clang-tidy/readability-redundant-smartptr-get.cpp
===
--- test/clang-tidy/readability-redundant-smartptr-get.cpp
+++ test/clang-tidy/readability-redundant-smartptr-get.cpp
@@ -9,13 +9,15 @@
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 template 
 struct shared_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 }  // namespace std
@@ -28,6 +30,7 @@
   Bar* operator->();
   Bar& operator*();
   Bar* get();
+  explicit operator bool() const;
 };
 struct int_ptr {
   int* get();
@@ -110,6 +113,23 @@
   // CHECK-MESSAGES: uu.get() == nullptr;
   // CHECK-FIXES: bool bb = uu == nullptr;
 
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+  bb = u.get() ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = u.get() ? true : false;
+  // CHECK-FIXES: bb = u ? true : false;
+
   bb = nullptr != ss->get();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
@@ -146,10 +166,6 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
-  if (x.get());
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
-  // CHECK-MESSAGES: if (x.get());
-  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
@@ -175,4 +191,6 @@
 
   int_ptr ip;
   bool bb = ip.get() == nullptr;
+  bb = !ip.get();
+  bb = ip.get() ? true : false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.emplace_back(Callback);
+  }

jbangert wrote:
> sbenza wrote:
> > Why emplace_back instead of push_back?
> Changed to push_back.  Is there ever an advantage to using push_back over 
> emplace_back (the latter falls back to a copy constructor). 
push_back and emplace_back are equivalent when the value you are passing is 
already a T.
In that case, push_back should be used from the principle of using the least 
powerful tool that can do the job you want.
emplace_back allows for calls to explicit constructors, which makes it more 
powerful in ways that might be surprising to the reader.
Eg:

std::vector> v;
... many lines below ...
v.push_back(1);  // fails
v.emplace_back(1);  // works, but it didn't add a 1 to the vector.


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:213
+llvm::errs() << "Node " << Element.Value
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");

I don't know if stderr is the best place for this error output.
Maybe we should take a sink of some sort in the constructor.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:214
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");
+  }

I don't think this is ok.
afaik, llvm_unreachable leads to undefined behavior if it is reached in opt 
mode.
This error can be triggered from user input. We should not fail that way.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:227
+  if (NodeMap.count(FromId) == 0) {
+llvm::errs() << "Node to be replaced " << FromId
+ << " not bound in query.\n";

Same as above. This error can be triggered from user input.


https://reviews.llvm.org/D29621



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

[PATCH] D31160: [clang-tidy] Fix misc-move-const-arg for move-only types.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:76
   if (IsConstArg || IsTriviallyCopyable) {
+if (const CXXRecordDecl *R = Arg->getType()->getAsCXXRecordDecl()) {
+  for (const auto *Ctor : R->ctors()) {

Can we get this from R->hasTrivialCopyConstructor or 
R->hasNonTrivialCopyConstructor instead of iterating all constructors?


https://reviews.llvm.org/D31160



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