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

[Manuel Klimek]

On Mon, Aug 20, 2012 at 1:22 PM, Sean Silva <silvas at purdue.edu> wrote:
> I'm not following you.
>
> If a patch doesn't have enough information to uniquely identify the
> source file(s) that it refers to, it would be impossible to apply,
> defeating the purpose of having a patch in the first place.

Humans are very good at figuring out context - for a machine that's
often harder. Obviously not impossible, but the question is one of
whether it's worth to spend the effort, and what the opportunity cost
is.

>
> --Sean Silva
>
> On Mon, Aug 20, 2012 at 3: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...
>>
>> 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
>>>>