Page MenuHomePhabricator

[clang-format] Add MacroUnexpander.
Needs ReviewPublic

Authored by klimek on Sep 25 2020, 6:28 AM.

Details

Reviewers
sammccall
Summary

MacroUnexpander applies the structural formatting of expanded lines into
UnwrappedLines to the corresponding unexpanded macro calls, resulting in
UnwrappedLines for the macro calls the user typed.

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
360 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w1\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sammccall added inline comments.Oct 12 2020, 11:23 AM
clang/lib/Format/FormatToken.h
439

I can't really understand from the comment when this is supposed to be set, and there are no tests of it.

(The comment is vague: is a "parent" the inverse of FormatToken::Children here? Is this scenario when the parents in question are new, or their children are new, or both? What part of the code is "formatting", and why would it otherwise skip the children?)

clang/lib/Format/Macros.h
134

"matches formatted lines" probably describes the hard technical problem it has to solve, but not so much what it does for the caller: what the transformation is between its inputs and its outputs.

Is it something like:

Rewrites expanded code (containing tokens expanded from macros) into spelled code (containing tokens for the macro call itself). Token types are preserved, so macro arguments in the output have semantically-correct types from their expansion. This is the point of expansion/unexpansion: to allow this information to be used in formatting.

[Is it just tokentypes? I guess it's also Role and MustBreakBefore and some other stuff like that?]

142

I'm a bit confused by these arrows. It doesn't seem that they each point to an unwrappedline passed to addLine?

142

This example didn't really help me understand the interface of this class, it seems to be a special case:

  • the input is a single block construct (rather than e.g. a whole program), but it's not clear why that's the case.
  • the output (unexpanded form) consists of exactly a macro call with no leading/trailing tokens, which isn't true in general

If the idea is to provide as input the minimal range of lines that span the macro, we should say so somewhere. But I would like to understand why we're not simply processing the whole file.

147

this says "creates the unwrapped lines" but getResult() returns only one.
Does the plural here refer to the tree? Maybe just use singular or "the unwrappedlinetree"?

154

I get the symmetry between the expander/unexpander classes, but the name is making it harder for me to follow the code.

  • the extra compound+negation in the name makes it hard/slow to understand, as I'm always thinking first about expansion
  • the fact that expansion/unexpansion are not actually opposites completely breaks my intuition. It also creates two different meanings of "unexpanded" that led me down the garden path a bit (e.g. in the test).

Would you consider MacroCollapser? It's artificial, but expand/collapse are well-known antonyms without being the same word.

(Incidentally, I just realized this is why I find "UnwrappedLine" so slippery - the ambiguity between "this isn't wrapped" and "this was unwrapped")

164

I find this hard to follow.

  • again, the "match" part is an implementation detail that sounds interesting but isn't :-)
  • "from a macro" sounds like one in particular, but is actually every macro
  • the bit about "getResult" is a separate point that feels shoehorned in

What about:
"Replaces tokens that were expanded from macros with the original macro calls. The result is stored and available in getResult()"

175

how can this be the case if the input can have multiple lines and the output only one?

Is the return value a synthetic parent of the translated lines?
Or is there a hidden requirement on the caller here that we don't keep feeding in lines unless we're continuing a macro call and therefore know we'll end up with one line?

This stuff could be clarified in docs but again I have to ask, can't we sidestep the whole issue by processing the whole file and returning all the lines?

(This is somewhat answered in the implementation, though that doesn't seem like the right place)

237

The explanation here seems to be proving the converse: if we *didn't* use this representation, then the code wouldn't work. However what I'm missing is an explanation of why it is correct/sensible.

After staring at the tests, I'm still not sure, since the tests seem to postprocess the "natural" output the same way before asserting equality.

My tentative conclusion is it would be clearest to move this "in the end" step to the *caller* of getResult(), as it seems to have more to do with formatting than unexpansion. But I haven't looked in detail at that caller, maybe I'm wrong...

clang/unittests/Format/MacroUnexpanderTest.cpp
2

All of these tests use both the expander and unexpander (so need consider behavior/bugs of both).
Is it possible to test the unexpander in isolation?

(Context: I'm trying to use the tests to understand what the class does, but the inputs aren't visible)

2

Having read all the tests here, they seem to follow exactly the same schema:

  1. some macro definitions
  2. some sequence of expand() calls, simulating expanding some macro-heavy code
  3. verify the expanded token sequence, rearranging it into expected UnwrappedLine structure
  4. call unexpand() and check that the result has the expected UnwrappedLine structure

You do this using various fairly-general helpers and DSLs, but don't really combine them in different ways... the tests are somewhat readable and error messages are OK, but if these are important tests, it might be worth looking at a data-driven test. e.g.

Case NestedChildBlocks;
NestedChildBlocks.Macros = {"ID(x)=x", "CALL(x)=f([] { x })"};
NestedChildBlocks.Original = "ID(CALL(CALL(return a * b;)))";
NestedChildBlocks.Expanded = R"cpp(
{
f([] {
  f([] {
    return a * b;
  })
})
}
)cpp"; // indentation shows structure
NestedChildBlocks.Unexpanded = R"cpp(
ID(
  {
  CALL(CALL(return a * b;))
  }
)
)cpp";
NestedChildBlocks.verify();

Definitely involves a bit of reinventing wheels though.

21

docs for this class/major members?

29

this name (and all the other "Unexpanded*" in this class) is misleading, because there's a process called "unexpanding" but these aren't the output of it.

I'd suggest "spelled", though I do think renaming the unexpander would also be worthwhile.

94

this needs docs (it's a cool technique! no need to make the reader decipher it though)

97

you always consume(lex(...)) - taking a StringRef directly instead would make it clearer that the identity of the temporary tokens doesn't matter

126

create() is a strange name for the *expander* in an *unexpander* test

142

FWIW, this seems confusing to me - line() has overloads that are simple, and then this one that does the same nontrivial stitching that the production code does.
The fact that the a/b lines are not sibling sub-lines in line({tokens("a"), tokens("b")})but they *are* sibling tokens in line(lex("a b")) is hard to keep track of in the tests.

If the stitching really is necessary, I think it's important for the expected output to also be shown in its stitched form.

156

why is this "tokens" and not "chunk"?

208

you have lots of assertions that this is true, but none that it is false

243

the high-level point of this test seems to be that by looking the post-expansion context, we can determine that b is a declared pointer so we don't put a space before it.

And everywhere these tokens are mentioned/verified here, the spacing is correct, as if it were propagated... but of course the spacing is actually ignored everywhere and underneath it's just sequences of tokens.
This makes it hard to track how well this it testing what it really wants to test.

Is it possible to mark the asterisk with the correct tokentype at the point where formatting would occur, and then verify that the unexpanded form (i.e. Chunk1.Tokens[1]) still has the tokentype set?
(It's probably possible to prove this by inspection by understanding what U1 contains, how Matcher works etc, but I think it'd still be illustrative)

246

what does id() mean if not identifier?

407

because this example isn't realistic, it'd be nice to have the comment here showing what formatting you're simulating

klimek updated this revision to Diff 301253.Oct 28 2020, 5:37 AM
klimek marked 3 inline comments as done.

Adapted based on review comments.

clang/lib/Format/FormatToken.h
439

Rewrote comment.

clang/lib/Format/Macros.h
134

It's not the token info, this we'd trivially have by using the original token sequence which is still lying around (we re-use the same tokens).

Reworded.

142

Why not? That is the intention? Note that high-level we do not pass class definitions in one unwrapped line; if they ever go into a single line it's done in the end by a special merging pass (yes, that's a bit confusing).

142

Re: input is a single construct - that is not the case (see other comment)
Re: leading / trailing tokens: I didn't want to make it too complex in the example.

147

Fixed comment.

154

Happy to rename, but first wanted to check in - I use "unexpanded token stream" quite often to refer to the macro call. Perhaps we should also find different wording for that then?

Perhaps we should call this MacroLineMatcher btw? This is not creating anything new - the resulting token sequence is the "unexpanded token sequence" with the exact same tokens, the special thing is that they're matched into unwrapped lines.

164

I think the match part is important, as it's matching unwrapped lines, which is the heart of the algorithm.

175

Reworded; the reason why we have the single-line anyway is that:

  • a macro call is something we generally want to have in one unwrapped line
  • the tokens (or other macro calls) that go into the same instance of MacroUnexpander only consist of tokens that do not have an unwrapped line break and macro calls

Thus, we want the output to be in a single unwrapped line, as we're otherwise majorly confusing ~everything else in the formatter.

237

This is basically what I wrote before - in the end, that the expanded code creates multiple unwrapped lines is the weird thing, as the input is fundamentally a single unwrapped line (the macro call plus a bit of stuff around it). Thus, it's quite natural for the unexpander to return a single unwrapped line that represents the original structure. Not sure how to best put this in words.

clang/unittests/Format/MacroUnexpanderTest.cpp
2

Not sure I understand what you mean - everything that's interesting about the inputs is spelled out in the test - namely, what the structure of unwrapped lines going into the unexpander is.

2

It seems like the main thing this does is replacing the structure how we build unwrapped lines with a DSL that gets parsed into unwrapped lines by indentation? I personally find that significantly less readable unless we'd create really good error messages if the indentation doesn't line up. In your example "Unexpanded" I have problems understanding exactly what goes into what unwrapped line intuitively - for example, {} can be one unwrapped line or 2 different ones. How do we decide when an unwrapped line is finished?

29

This comment made it clear why Unwrapper is a really bad name, because it's also not about collapsing.

97

Yeah, I thought that I want Matcher and the test to be more decoupled, but passing in the Lexer is not a big thing, so changed.

142

Would renaming tokens() to chunk() help with that? The idea is that in the test I mostly need to create a single line from chunks of tokens. Also happy to rename this function if it helps more?

156

My thought was that Chunk is just a type for a chunk of a line, and I can create one from a set of tokens (via tokens()) or from a set of child unwrapped lines (via children()).

208

Yeah, that's a white-box assertion - finished() is false the vast majority of the time, so testing the true cases is the important part - happy to add tests if you think it's worth it.

243

I don't care about anything about the tokens than that they have pointer identity and that the same tokens end up in the right unwrapped lines in the result (thus match also checks token identity).

246

Sigh, yeah, good point - initially this was used for identifiers, but it really means "lex exactly one token". Do you have a good name that is short (it's used really often) and descriptive?

407

As before, formatting is (to me) not relevant here - what's relevant is that we don't crash and the resulting unwrapped lines contain the right tokens.

Thanks a lot for all the time explaining, I get this at least at a high level now.
I have trouble following the ins and outs of the algorithm, but I think I understand most of it and tests cover the rest.

Regarding tests: I have trouble reasoning about whether all cases are tested. I wonder if we can easily throw this against (internal) coverage + mutation testing infrastructure? I'm not a massive believer in that stuff usually, but this seems like a good candidate.

Lots of comments around making this easier for confused people like me to understand, partly because I don't understand it well enough to suggest more substantive changes.
Throughout, I think it'd be nice to be explicit about:

  • which structure tokens/lines are part of: spelled/expanded/unexpanded. (Including getting rid of input/output/incoming/original, which AFAICT are synonyms for these)
  • which lines are complete, and which are partial (being added to as we go)

It took me a while to understand the control flow and to tell "who's driving" - I think it was hard for me to see that processNextUnexpanded was basically a leaf, and that unexpand/startUnexpansion/endUnexpansion/continueUnexpansionUntil were the layer above, and add() was the top. Maybe there's naming that would clarify this better, but I don't have great suggestions, and I'm not sure if this is a problem other people would have.
Maybe an example trace showing the internal method calls when parsing a small example might help... but again, not sure.

clang/lib/Format/MacroUnexpander.cpp
52

this doesn't use any state, right?

it could be static or even private to the impl file.

replacing std::function with a template param allow this loop to be specialized for the one callsite - up to you, maybe it doesn't matter much, but it's not very invasive

66

I find using all of spelled/expanded/unexpanded, input/incoming/output/outgoing, and original, unclear.
(Particularly after OriginalParent is propagated around by that name without comment, and because "original" refers to the expanded structure, which is *not* the first one created)
Can we call this "ExpandedParent in the expanded unwrapped line"?

66

Unexand -> unexpand

77

I can't work out what "it" refers to in this sentence.

(and "spelled" token stream?)

80

(in these examples throughout, #define BRACED(a) {a} would make it clearer that this is a macro definition and not code)

82

It would be helpful to complete the example by spelling out which token you're adding, which the correct parent is, and which tokens you need to "expand over" to make it available.

I *think* the answer to the first two is - when you're adding the a then its proper parent is the inner ( in BRACED(BRACED(... but I don't know the last part.

84

is it possible that you may need to unexpand more than the innermost macro?

e.g. BRACED(ID(ID(BRACED(ID(ID(a)))))) expands to {{a}} and the parents of a and inner { each come from a couple of macro-levels up.

(I don't totally understand the logic here, so the answer's probably "no"...)

103

I had trouble understanding the what this function does at a high level: i.e. *why* you'd want this.

Maybe:

Adjusts the stack of active (unexpanded) lines so we're ready to push tokens.
The tokens to be pushed are children of ExpandedParent (in the expanded code).
This may entail:
 - creating a new line, if the parent is on the active line
 - popping active lines, if the parent is further up the stack

and s/First/ForceNewLine/ to avoid documenting it?

(impl looks good though once I understood what it does!)

156

nit: s/find/lookup/, then you only have to deal with pointers and this becomes a bit easier to read IMO

173

This raises another point: a macro can have an empty body.
AFAICT this fundamentally isn't supported here, as we're driven by the expanded token stream.
I guess it makes sense to handle this elsewhere in clang-format (or even not handle it) but it should be documented somewhere.

237

assert that this number is equal to StartOfExpansion?

238

nit: index arithmetic obscures what's going on a bit.
You could write

ArrayRef<FormatToken *> StartedMacros = makeArrayRef(Token->MacroCtx->ExpandedFrom).drop_front(Unexpanded.size());
for (FormatToken *ID : llvm::reverse(StartedMacros)) {
...
}

but up to you

It's not obvious to me *why* we're iterating in reverse order BTW: i.e. why the order of the Unexpanded stack is opposite the order of the ExpandedFrom stack.
So maybe just a comment to reinforce that, like (// ExpandedFrom is outside-in order, we want inside-out)

263

maybe unexpandActiveMacroUntil or so?

Something to hint that this stops at the end of the top-of-stack macro.

(and to avoid the term "unexpansion stream" which isn't used/defined anywhere else)

269

nit: while

273

nit: this assert is just the opposite of the while condition, drop it?

282

nit: Token->MacroCtx is checked at the callsite, and startUnexpansion asserts it as a precondition - do that here too for symmetry?

303

finishe -> finish

310

I can't work out if this is supposed to say comma or comment :-)

If comma - is that a thing?
If comment - why would a comment be considered part of the unexpansion of a macro invocation, rather than a sibling to the macro? How do we know what trails is a comment - should we have an assertion?

319

describe the return value, it's not obvious

322

want to keep this?

345

nit: easier to see the three cases being handled independently (comma, rparen, lparen) if they're all siblings instead of grouping two together.

348

This line is both subtle and cryptic :-).

// New unwrapped-lines from unexpansion are normally "chained" - treated as
// children of the last token of the previous line. 
// Redirect them to be treated as children of the comma instead.
// (only affects lines added before we push more tokens into MacroStructure.Line)
366

This feels like a stupid question, but how do we know that this non-macro-parenthesis has anything to do with macros?
(Fairly sure the answer is "processNextUnexpanded() is only called when the token is macro-related", it'd be nice to have this precondition spelled out somewhere)

367

please assign to the struct fields or use /*Foo=*/ comments

All the data flow around this struct is local but the order of fields isn't :-)

369

nit: = MacroCallStructure.back().RedirectParentTo
This line + line 361 are the keys to understanding what the RedirectParentFrom/To do, so it'd be helpful for them to explicitly use those variables.

(or use the source expressions for both... but in that case the fields may need docs)

391

nit: finished()

396

nit: pull out a named reference for Output.Tokens.front() after the size assertion.

assert(NullToken.is(tok::null) && !NullToken.children.empty()) may even obviate the need for a comment :-)

398

resulting

406

hmm, when is this possible? are these literal blank lines? where do they come from?

409

Isn't this just the end of line from the previous iteration of this loop? Why the global map populated by pushToken?
(I feel like i'm missing something obvious here)

411

nit: again .lookup()

423

nit: i'd consider calling this appendToken, since lots of the stuff in this class deals with stacks but this doesn't)

(I say a lot of nasty things about java but ArrayList.add is 1000x better name than vector::push_back)

clang/lib/Format/Macros.h
28

This would be a good place to explicitly introduce the name "unexpanded" for what comes out of the unexpander.

This para gives names to the token streams, but not as clearly to the UnwrappedLines parsed out of them.

I think the fact that the tokens *alias* between the streams/lines, and so the final formatting of the expanded lines "writes through" tokentype etc to the unexpanded lines, is an important design point worth emphasizing.

(This part is mostly structure around "what happens", with the data sets secondary - I think I'd personally find the reverse easier to follow but YMMV)

30

would s/formats/annotates/ be inaccurate?

This is just my poor understanding of the code, but it wasn't obvious to me that annotation is closely associated with formatting and not with parsing UnwrappedLines.

135

I know it's the common case, but I think saying "the macro call" here is misleading, because it quickly becomes apparent reading the code that the scope *isn't* one macro call, and (at least for me) it's easy to get hung up on not understanding what the scope is. (AIUI the scope is actually one UL of *output*... so the use of plural there is also confusing).

I think a escription could be something like:

Converts a sequence of UnwrappedLines containing expanded macros into a single UnwrappedLine containing the macro calls.
This UnwrappedLine may be broken into child lines, in a way that best conveys the structure of the expanded code.
...
In the simplest case, a spelled UnwrappedLine contains one macro, and after expanding it we have one expanded UnwrappedLine.
In general, macro expansions can span UnwrappedLines, and multiple macros can contribute tokens to the same line.
We keep consuming expanded lines until:

  • all expansions that started have finished (we're not chopping any macros in half)
  • *and* we've reached the end of a *spelled* unwrapped line

A single UnwrappedLine represents this chunk of code.

After this point, the state of the spelled/expanded stream is "in sync" (both at the start of an UnwrappedLine, with no macros open), so the Unexpander can be thrown away and parsing can continue.

(and then launch into an example)

142

Re: input is a single construct - that is not the case (see other comment)

A class is definitely a single construct :-) It sounds like that's not significant to the MacroUnexpander though, so it feels like a red herring to me.

Re: leading / trailing tokens: I didn't want to make it too complex in the example.

That seems fine, I think the complexities of the general case need to be mentioned somewhere because the API reflects them. But you're right, the primary example should be simple.
I think a tricky example (maybe the #define M ; x++ one?) could be given on one of addLine/finish/getResult maybe.

154

I think unexpanded/unexpander are reasonable names, having understood this better, but with caveats.

It's important to distinguish between the pre-expansion state ("spelled"?) and the post-unexpansion state ("unexpanded?").
The UnwrappedLines are vitally different, but the token stream is the same as you point out.

When referring to the token stream, I think "spelled" is probably less confusing (that's where the token stream fundamentally comes from), and explicitly mentioning somewhere that the token sequence encoded by the unexpanded lines is the same original spelled stream.

163

any reason for std::map rather than DenseMap, here and elsewhere? (Only good reasons i'm aware of are sorting and pointer stability)

174

const? or do we not care

174

Maybe comment that when finished() is true, it's possible to call getResult() and stop processing... but also valid to continue calling addLine(), if this isn't a good place to stop.

182

Maybe a note like "this representation is chosen because it can be opaque to the UnwrappedLineParser, but the Formatter treats it appropriately" or something.
I think it should be clear that this representation isn't really "natural" at this layer (or if it is, why).
Maybe an example would help.

189

ASCII art of some sort would help :-)

192

nit: giving getResult() a side-effect but also making it idempotent is a bit clever/confusing.

Either exposing void finalize(); + UnwrappedLine get() const, or UnwrappedLine takeResult() &&, seems a little more obvious.

218

you have this "no expansions are open, but we already didn't find any" state.
The effect of this is that finished() returns false so the caller keeps looping.

But a correct caller will never rely on this:

  • the first line a caller feeds must have macro tokens in it, or our output will be garbage AFAICT
  • calling getResult() without feeding in any lines is definitely not correct

It seems we could rather assert on these two conditions, and eliminate the Start/InProgress distinction.
That way incorrect usage is an assertion crash, rather than turning into an infinite loop.

220

similarly, the InProgress/Finalized distinction would be eliminated by making takeResult() destructive, and requiring (through the type system or an assert) that it be called only once.
It doesn't seem that it needs to be part of the NDEBUG runtime control flow.

249

This is very closely related to what you return from "getResult" - not quite the same, but Output vs Result doesn't seem to hint at the difference. Could we use the same name for both?

253

ActiveUnexpandedLines?

(Line is very overloaded here)

278

ParentInExpandedToParentInUnexpanded?

(current name implies that it maps a token to *its* parent. It also uses the input/output names, rather than expanded/unexpanded - it would be nice to be consistent)

286

consider calling these NextSpelled and EndSpelled to be really explicit?
Since the type doesn't really clarify which sequence is being referred to.

291

I think this is a confusing use of "unexpanded".

These macros that we're in the process of unexpanding. So the past tense doesn't seem right, they don't seem more "unexpanded" than they do "expanded", at least to me.

Maybe ActiveExpansions or so?

314

nit: if you're going to specify SmallVector sizes, I don't understand why you'd size Unexpanded vs MacroCallStructure differently - they're going to be the same size, right?

These days you can leave off the size entirely though, and have it pick a value

clang/unittests/Format/MacroUnexpanderTest.cpp
202

There's no test for a macro that expands to nothing - is this supported?

208

Yes - a basic test that it's not always true would be useful I think (maybe the #define M ;x++ case would be useful for showing the expected loop and finished() values)

541

nit: Result