[cfe-commits] [PATCH] New matcher for MaterializeTemporaryExpression.

[David Blaikie]

On Mon, Aug 20, 2012 at 12:58 PM, Manuel Klimek <klimek at google.com> wrote:
> On Mon, Aug 20, 2012 at 12:37 PM, Sean Silva <silvas at purdue.edu> wrote:
>> Phabricator feedback:
>>
>> Would it be possible for the "table of contents" (or somewhere else
>> obvious on the page, like maybe near the diffs) to contain links to
>> the original source code of the file, and not just the diff? I felt
>> myself wanting to browse the whole ASTMatchers.h but in not finding a
>> link in the end I fired up a terminal to look at it.
>
> Unfortunately that's hard in general, because the patch might contain
> arbitrarily small subsets at the end of the real path which we'd need
> to match up with the repo...

I'd say even filenames (no path context) would be fairly unique - and
the functionality could simply not work if the path was ambiguous
(heroic efforts: if the path was ambiguous and the patch context
didn't apply to any of the files).

I suppose another issue would be figuring out which revision to
compare to (I guess the subversion diff mentions "revision N" in the
diff & git diffs have some of the hash value in them, it seems - not
sure if phab has the smarts to cross reference multiple source control
systems like that, though).

> Fortunately we have a workaround: uploading patches with a large
> context setting (so that the whole file is included in the uploaded
> diff) fixes the immediate problem, and phab is smart enough to output
> and attach a small-context patch on the review mail.
>
> Cheers,
> /Manuel
>
>>
>> Thanks,
>>
>> --Sean Silva
>>
>> On Mon, Aug 20, 2012 at 2:36 PM, Sam Panzer
>> <reviews at llvm-reviews.chandlerc.com> wrote:
>>> Hi klimek,
>>>
>>> A new matcher which corresponds to the MaterializeTemporaryExpression AST node.
>>>
>>> http://llvm-reviews.chandlerc.com/D20
>>>
>>> Files:
>>>   include/clang/ASTMatchers/ASTMatchers.h
>>>   unittests/ASTMatchers/ASTMatchersTest.cpp
>>>
>>> Index: include/clang/ASTMatchers/ASTMatchers.h
>>> ===================================================================
>>> --- include/clang/ASTMatchers/ASTMatchers.h
>>> +++ include/clang/ASTMatchers/ASTMatchers.h
>>> @@ -473,6 +473,22 @@
>>>    Stmt,
>>>    CXXBindTemporaryExpr> bindTemporaryExpression;
>>>
>>> +/// \brief Matches nodes where temporaries are materialized.
>>> +///
>>> +/// Example: Given
>>> +///   struct T {void func()};
>>> +///   T f();
>>> +///   void g(T);
>>> +/// materializeTempExpr() matches 'f()' in these statements
>>> +///   T u(f());
>>> +///   g(f());
>>> +/// but does not match
>>> +///   f();
>>> +///   f().func();
>>> +const internal::VariadicDynCastAllOfMatcher<
>>> +  Stmt,
>>> +  MaterializeTemporaryExpr> materializeTempExpr;
>>> +
>>>  /// \brief Matches new expressions.
>>>  ///
>>>  /// Given
>>> Index: unittests/ASTMatchers/ASTMatchersTest.cpp
>>> ===================================================================
>>> --- unittests/ASTMatchers/ASTMatchersTest.cpp
>>> +++ unittests/ASTMatchers/ASTMatchersTest.cpp
>>> @@ -1249,6 +1249,43 @@
>>>                   TempExpression));
>>>  }
>>>
>>> +TEST(MaterializeTempExpr, MatchesTemporary) {
>>> +  StatementMatcher MaterializeTemp = materializeTempExpr();
>>> +
>>> +  std::string ClassString =
>>> +      "class string { public: string(); int length(); }; ";
>>> +
>>> +  EXPECT_TRUE(
>>> +      matches(ClassString +
>>> +              "string GetStringByValue();"
>>> +              "void FunctionTakesString(string s);"
>>> +              "void run() { FunctionTakesString(GetStringByValue()); }",
>>> +              MaterializeTemp));
>>> +
>>> +  EXPECT_TRUE(
>>> +      notMatches(ClassString +
>>> +                 "string* GetStringPointer(); "
>>> +                 "void FunctionTakesStringPtr(string* s);"
>>> +                 "void run() {"
>>> +                 "  string* s = GetStringPointer();"
>>> +                 "  FunctionTakesStringPtr(GetStringPointer());"
>>> +                 "  FunctionTakesStringPtr(s);"
>>> +                 "}",
>>> +                 MaterializeTemp));
>>> +
>>> +  EXPECT_TRUE(
>>> +      notMatches(ClassString +
>>> +                 "string GetStringByValue();"
>>> +                 "void run() { int k = GetStringByValue().length(); }",
>>> +                 MaterializeTemp));
>>> +
>>> +  EXPECT_TRUE(
>>> +      notMatches(ClassString +
>>> +                 "string GetStringByValue();"
>>> +                 "void run() { GetStringByValue(); }",
>>> +                 MaterializeTemp));
>>> +}
>>> +
>>>  TEST(ConstructorDeclaration, SimpleCase) {
>>>    EXPECT_TRUE(matches("class Foo { Foo(int i); };",
>>>                        constructor(ofClass(hasName("Foo")))));
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits