This is an archive of the discontinued LLVM Phabricator instance.

[libTooling] Introduce new library of source-code builders.
ClosedPublic

Authored by ymandel on Sep 16 2019, 1:57 PM.

Details

Summary

Introduces facilities for easily building source-code strings, including
idiomatic use of the address-of, dereference and member-access operators (dot
and arrow) and queries about need for parentheses.

Event Timeline

ymandel created this revision.Sep 16 2019, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 1:57 PM
Herald added a subscriber: mgorny. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp
77

Could return {}. Same in other places. By the word, did you run Clang-format over code?

ymandel updated this revision to Diff 220488.Sep 17 2019, 5:35 AM

ran clang-format

ymandel marked an inline comment as done.Sep 17 2019, 5:37 AM
ymandel added inline comments.
clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp
77

No, thanks for pointing that out.

I find return std::string() more readable, because it doesn't require that I check/know the return type of the function. But, if return {} is standard, I'm fine changing it. What's most common in the clang source?

ymandel updated this revision to Diff 220901.Sep 19 2019, 1:05 PM

Added parens operator; cleaned comments and api a bit.

gribozavr added inline comments.Sep 20 2019, 5:45 AM
clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h
10

"\file"

11

"source code"

31

s/a//

or s/a parentheses/a pair of parentheses/

34

mayEverNeedParens? (to emphasize conservativeness and lack of context)

57

Given that empty string is returned in a vanishingly small number of cases, it would be worth it returning llvm::Optional<std::string> to force the caller to handle the failure.

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp
22

No need to repeat the comment from the header.

69

buildDereference below checks for getText returning an empty string; this function does not. Why?

77

I personally find return ""; more readable. After all, we don't care about the specific type, we want to express that we return an empty string. The most natural way to write an empty string is a string literal "".

clang/unittests/Tooling/SourceCodeBuildersTest.cpp
24

"translation unit"

77

Also need tests for:

  • overloaded binary operator S(10) + S(20)
  • implicit conversions void takeS(S); takeS(10 + 20);
ymandel marked an inline comment as done.Sep 20 2019, 6:32 AM

Thanks for the review! Agreed on all points and then some -- next revision will have a bunch of cleanups. I think the only major issue is the return type. See below.

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp
69

Regarding this and the issue of returning llvm::Optional: the code overall is rather inconsistent in its handling of empty strings returned from getText. This is only one example.

sorry for this -- this code was mostly collected from various other places in the codebase and I should have looked it over more carefully.

With that said -- I think we need to decide whether we systematically assume that getText doesn't return an empty string, or systematically check for it and return llvm::None in that case. We have the choice becuase there is no risk of UB from ignoring the empty text. Any checks in the code seem only designed to propagate the "emptiness" for the caller's sake.

I'm inclined to state a precondition on the all of the functions that getText(E) is non empty, rather than sprinkle optionals throughout the API. My reasoning is that checking the result of these functions is "too late" in the process. A failure here is caused by passing a node that has no corresponding source code. If the caller is potentially dealing with such phantom nodes, they should know that before trying to construct code with them. Any check they can do on the result of getText they should have done earlier with a more appropriate predicate of the node.

That said, I understand the preference for defensiveness (don't assume the caller will get it right). I'm also fine with a compromise of asserting the non-emptiness of any internal call to getText although that might be almost as messy as just handling it.

WDYT?

ymandel updated this revision to Diff 221041.Sep 20 2019, 8:30 AM

address comments and some general cleanup

gribozavr added inline comments.Sep 20 2019, 11:36 AM
clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp
69

It is true that there is no risk of UB (in terms of C++ spec), but there is a garbage-in/garbage-out issue. I don't think users will appreciate garbage out, because I don't think they have the ability to check the precondition properly -- after all, they don't know how exactly getText will be called.

Even if we figure out how to specify for users to check the precondition, they would always have to check it in order to use these APIs properly. After all, the source transformation tools that users write can't generally assume what code *their users* will throw at those tools. And if someone wants to throw together a simple tool prototype, they can always force-unwrap the optional, it is just an extra *.

Therefore, I believe embedding the check in these APIs will create a significantly better usage experience, and does not create much of a burden for users who don't want to handle errors.

ymandel updated this revision to Diff 221103.Sep 20 2019, 1:25 PM

converted builders to return optional

ymandel marked 12 inline comments as done.Sep 20 2019, 1:27 PM
gribozavr accepted this revision.Sep 23 2019, 1:24 AM
This revision is now accepted and ready to land.Sep 23 2019, 1:24 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 5:38 AM