Issue 6303052: Implemented macro expansion.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1148 lines, -13 lines)			Patch
M	src/compiler/preprocessor/new/Diagnostics.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/compiler/preprocessor/new/DirectiveParser.cpp	View	1	4 chunks	+8 lines, -6 lines	0 comments	Download
M	src/compiler/preprocessor/new/Macro.h	View	1 2	1 chunk	+6 lines, -2 lines	0 comments	Download
M	src/compiler/preprocessor/new/MacroExpander.h	View	1 2 3	2 chunks	+37 lines, -0 lines	0 comments	Download
M	src/compiler/preprocessor/new/MacroExpander.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+321 lines, -2 lines	0 comments	Download
M	src/compiler/preprocessor/new/Token.h	View		2 chunks	+7 lines, -3 lines	0 comments	Download
M	src/compiler/preprocessor/new/Token.cpp	View		1 chunk	+8 lines, -0 lines	0 comments	Download
M	src/compiler/preprocessor/new/Tokenizer.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/compiler/preprocessor/new/Tokenizer.l	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	tests/build_tests.gyp	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	tests/preprocessor_tests/define_test.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+755 lines, -0 lines	0 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

Alok Priyadarshi

I promise this will be the last complicated code review for you. Macro expansion is ...

11 years, 11 months ago (2012-06-11 22:38:03 UTC) #1

kbr1

This is just superb. Excellent work. The tests are fantastic. It would be great to ...

11 years, 11 months ago (2012-06-13 21:38:02 UTC) #2

Alok Priyadarshi

11 years, 11 months ago (2012-06-14 18:29:15 UTC) #3

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
File src/compiler/preprocessor/new/DirectiveParser.cpp (right):

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
src/compiler/preprocessor/new/DirectiveParser.cpp:168:
macro.parameters.push_back(token->value);
On 2012/06/13 21:38:02, kbr1 wrote:
> I looked through this loop again and don't understand how it's handling some
of
> the cases in your tests such as MACRO(red fish, blue fish). But it's excellent
> that it does.

Here we are parsing the #define directive, not the macro invocation. So we are
only collecting the macro parameters (which are identifiers), not the actual
arguments.

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
File src/compiler/preprocessor/new/MacroExpander.cpp (right):

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
src/compiler/preprocessor/new/MacroExpander.cpp:128: context->unget();
On 2012/06/13 21:38:02, kbr1 wrote:
> Does this code need to be robust to the case where the incoming token differs
> from the last one fetched from the MacroContext? ("No" is a fine answer.)

The following asserts make sure that the incoming token is the same.

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
src/compiler/preprocessor/new/MacroExpander.cpp:288: // Pre-expand each argument
before substitution.
On 2012/06/13 21:38:02, kbr1 wrote:
> This seems subtle. Could you provide more information about what this step
does?
> Also, does this code handle any necessary recursion (looks like it does, since
> it uses another MacroExpander instance, but just checking)?

Yes it does handle recursion. Added comments.

http://codereview.appspot.com/6303052/diff/6012/src/compiler/preprocessor/new...
src/compiler/preprocessor/new/MacroExpander.cpp:339: // macro replacement token.
On 2012/06/13 21:38:02, kbr1 wrote:
> Just the first one? Not all of them? Noticed similar code earlier in the
review.

Here we are inserting the argument into the macro body. All intermediate tokens
already have fixed padding properties. Only the first one should inherit that
from the macro body.

http://codereview.appspot.com/6303052/diff/6012/tests/preprocessor_tests/defi...
File tests/preprocessor_tests/define_test.cpp (right):

http://codereview.appspot.com/6303052/diff/6012/tests/preprocessor_tests/defi...
tests/preprocessor_tests/define_test.cpp:335: "two(,)\n";
On 2012/06/13 21:38:02, kbr1 wrote:
> Is that really legal?

Yes! When a macro is called without an argument, it can mean one (empty token or
space token) or zero argument.

http://codereview.appspot.com/6303052/diff/6012/tests/preprocessor_tests/defi...
tests/preprocessor_tests/define_test.cpp:421: "\n";
On 2012/06/13 21:38:02, kbr1 wrote:
> Is this really how this behaves?

This test is testing whether macro invocation spanning multiple lines can be
correctly parsed. I changed bar to (a) to make it clearer.

http://codereview.appspot.com/6303052/diff/6012/tests/preprocessor_tests/defi...
tests/preprocessor_tests/define_test.cpp:625: "b*2\n";
On 2012/06/13 21:38:02, kbr1 wrote:
> Wow, that works?

Yup. It does.

Expand All Messages | Collapse All Messages