Fix preprocessor expansion strategy #33

doppioandante · 2021-09-26T14:21:46Z

Fixes #31
Fixes #30
Supersedes #29
Still WIP, I need to reintegrate the logic needed to handle __VA_ARGS__ and some other things, but overall it works.

I will tidy up the commits when done.

doppioandante · 2021-09-27T13:57:05Z

@Vexu this is still not ready for a full review, but I have to suspend working on this for a week or so. Could you give it a quick skim regarding structure and memory handling? It's practically the first real piece of zig code I write, and I felt it was rough a lot of times.

The entry point is expandMacro2(), if one substitutes it to expandMacro() in the two occurrences in the source, the new code will be used. I have added new test cases and everything passes now, but I have some regressions which aren't being tested and a lot of things I want to fix first.

Vexu

So far this seems mostly fine, I tried answering most of the questions in the TODOs.
I'd be nice if more allocations could be avoided but correctness comes first.

test/cases/unspecified expansion.c

src/Preprocessor.zig

doppioandante · 2021-10-02T10:46:51Z

Small status update, don't review yet because the commits are a mess right now, It seems that the preprocessor is working ok. I've tested it with libpp and it passes all its tests. I also tested it with cloak, but that didn't work completely, I suspect it relies on the undefined behaviour that arocc does differently respect to gcc/clang.

I will test it with more libraries from awesome-c-preprocessor

A note about commit cf776f3 . That commit deals with the following:

#define h(s) 30
#define H h
#define SS (s)

H SS // should this be h(s) or 30?

gcc/clang expand as h(s), while arocc before cf776f3 expands to 30, after it does like gcc. What gcc does is reasonable, but I'm not sure it is strictly mandated by the standard

doppioandante · 2021-10-02T19:55:15Z

@Vexu I'm stopping here with the fixes and enhancements, this is getting a bit out of hand already.
Can you please tell me if you prefer to review this after I have rebased/reordered the commits or if you want to review now? Being this big, it is a bit easier to be able to see the timeline of the changes, but it's the same for me.

After the rebasing, I will add documentation to all the parts I touched, and I will do following development in future PR:

remove .empty from the Macro enum and rework the func/simple distinction
make the location linked list a vector
FIx the commented parts of test test/cases/standard-concatenation-strings-example.c
A bit of explanation on the last one: this is what fails now

#define str(x) #x
str(f(x)+1)
str(f(x) + 1)

This should expand to "str(f(x)+1)" and "str(f(x) + 1)" respectively. The previous implementation always did the second, the current always does the former.
As far as I can tell, the current preprocessor completely deletes simple spaces between token, is that correct? Unfortunately I think those must be kept track of, what do you think about this?

src/Preprocessor.zig

Vexu · 2021-10-03T06:41:17Z

Can you please tell me if you prefer to review this after I have rebased/reordered the commits or if you want to review now?

I can review it now, should make squashing any fixups easier too.

Unfortunately I think those must be kept track of, what do you think about this?

Yes that seems to be the case, it should also make properly implementing the -E command easier and shouldn't add too much memory usage as we don't have to pass the whitespace tokens out of the preprocessor and they don't need proper source locations.

src/Preprocessor.zig

Vexu

This change looks good and removes some of the ugliest parts of the entire codebase.

I didn't make notes about all the debug comments and prints since you'll probably remove them as part of the rebasing.

test/cases/macro re-expansion.c

test/cases/unspecified expansion.c

src/Preprocessor.zig

The previous macro expansion implementation had several flaws regarding order and re-expansion logic. Those were fixed by a complete rewrite of the expandMacro() and its nested code paths. Also remove special handling for 'self' macros. In the future, the 'empty' macro will be removed as well.

doppioandante · 2021-10-03T14:12:34Z

src/Preprocessor.zig

+                    try buf.replaceRange(idx, macro_scan_idx-idx+1, res.items);
+                    // TODO: moving_end_idx += res.items.len - (macro_scan_idx-idx+1)
+                    // doesn't work when the RHS is negative (unsigned!)
+                    moving_end_idx = moving_end_idx + res.items.len - (macro_scan_idx-idx+1);


Any suggestion about this? It was moving_end_idx -= res.items.len - (macro_scan_idx-idx+1);
But the rhs can become negative, which is harmless but zig doesn't like it. It works right now, but I don't know if this is the zig way

doppioandante · 2021-10-03T14:14:16Z

src/Preprocessor.zig

@@ -70,11 +70,7 @@ defines: DefineMap,
 tokens: Token.List = .{},
 generated: std.ArrayList(u8),
 token_buf: RawTokenList,
-char_buf: std.ArrayList(u8),


this was only used during stringification, so I moved it there locally, is that okay?

The reason it was in the PreProcessor was so that the buffer could be reused between stringifications redusing redundant allocations.

I reintroduced the global pp.char_buf

…s at beginning These modifications strive for compatibility with gcc, but I'm not sure it is strictly mandated by the standard

doppioandante · 2021-10-03T14:26:00Z

Rebased and applied zig fmt. I originally wanted to also add some docs to this PR, but maybe I'll leave that for another one if that's okay for you?
I left another couple of comments, but other than that I think it's good to go.

Mainly changes to the control flow of the macro expansion/rescan loop Co-authored-by: Veikka Tuominen <git@vexu.eu>

Vexu reviewed Sep 27, 2021

View reviewed changes

doppioandante mentioned this pull request Sep 27, 2021

Fix recursive macro func substitution #29

Closed

doppioandante force-pushed the preprocessor-fix-2 branch from 130cd37 to c0f0d4b Compare October 2, 2021 20:00

doppioandante commented Oct 2, 2021

View reviewed changes

src/Preprocessor.zig Outdated Show resolved Hide resolved

doppioandante force-pushed the preprocessor-fix-2 branch from c0f0d4b to 2a9e1b0 Compare October 2, 2021 20:15

doppioandante commented Oct 3, 2021

View reviewed changes

src/Preprocessor.zig Outdated Show resolved Hide resolved

Vexu approved these changes Oct 3, 2021

View reviewed changes

doppioandante force-pushed the preprocessor-fix-2 branch 3 times, most recently from 31da779 to 8083895 Compare October 3, 2021 13:29

doppioandante force-pushed the preprocessor-fix-2 branch from fab1e21 to dcb8477 Compare October 3, 2021 14:10

doppioandante commented Oct 3, 2021

View reviewed changes

doppioandante added 8 commits October 3, 2021 16:25

Preprocessor: add more test cases

201abc5

Preprocessor: handle token pasting when arguments are empty

3cbf372

Preprocessor: emulate gcc by not rescanning completely expanded token…

c71e994

…s at beginning These modifications strive for compatibility with gcc, but I'm not sure it is strictly mandated by the standard

Preprocessor: add more testcases

4b6f68b

Preprocessor: fix ## concatenation when used multiple times in sequence

6b54645

Preprocessor: don't put spaces between tokens inside stringification

498c30a

Preprocessor: escape \ during stringification

1c9469d

Preprocessor: skip newlines when searching for first macro call lparen

723cbd8

doppioandante force-pushed the preprocessor-fix-2 branch from 405b883 to 9e045e2 Compare October 3, 2021 14:25

doppioandante marked this pull request as ready for review October 3, 2021 14:27

Preprocessor: remove old preprocessor code path

6544dcf

doppioandante and others added 3 commits October 6, 2021 20:51

Preprocessor: apply suggestions from code review

bedd6ac

Mainly changes to the control flow of the macro expansion/rescan loop Co-authored-by: Veikka Tuominen <git@vexu.eu>

Preprocessor: remove debugging statements and functions

31449f4

zig fmt

e3e4ac4

doppioandante force-pushed the preprocessor-fix-2 branch from 9e045e2 to e3e4ac4 Compare October 6, 2021 18:53

Vexu merged commit 4befc7b into Vexu:master Oct 10, 2021

doppioandante deleted the preprocessor-fix-2 branch October 11, 2021 12:34

This was referenced Oct 11, 2021

[Preprocessor] Simplify the Macro union #37

Closed

[Source.zig] Change Source.Location from linked list to array #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix preprocessor expansion strategy #33

Fix preprocessor expansion strategy #33

doppioandante commented Sep 26, 2021 •

edited

Loading

doppioandante commented Sep 27, 2021

Vexu left a comment

doppioandante commented Oct 2, 2021

doppioandante commented Oct 2, 2021

Vexu commented Oct 3, 2021

Vexu left a comment

doppioandante Oct 3, 2021

doppioandante Oct 3, 2021

Vexu Oct 5, 2021

doppioandante Oct 6, 2021

doppioandante commented Oct 3, 2021

Fix preprocessor expansion strategy #33

Fix preprocessor expansion strategy #33

Conversation

doppioandante commented Sep 26, 2021 • edited Loading

doppioandante commented Sep 27, 2021

Vexu left a comment

Choose a reason for hiding this comment

doppioandante commented Oct 2, 2021

doppioandante commented Oct 2, 2021

Vexu commented Oct 3, 2021

Vexu left a comment

Choose a reason for hiding this comment

doppioandante Oct 3, 2021

Choose a reason for hiding this comment

doppioandante Oct 3, 2021

Choose a reason for hiding this comment

Vexu Oct 5, 2021

Choose a reason for hiding this comment

doppioandante Oct 6, 2021

Choose a reason for hiding this comment

doppioandante commented Oct 3, 2021

doppioandante commented Sep 26, 2021 •

edited

Loading