Add c++17 fold expression

[i-garrison]

No description provided.

[jonahgraham]

Added call for reviewers on cdt-dev. If none are forthcoming we'll need a plan-B (me learning this area?)

[ddscharfe]

• AST2TemplateTests::testFoldExpression1() fails on my machine on line 11426.
• I get Invalid overload of 'std::endl' for the following example, don't know if this is a use case you want to cover:

#include <iostream>

template<typename ... T>
void sum(T ... vals) {
	std::cout << "Total = " << (... + vals) << std::endl;
}

int main() {
	sum(1, 2, 3);
	return 0;
}

[i-garrison]

• AST2TemplateTests::testFoldExpression1() fails on my machine on line 11426.

Yes this test actually depends on pack expansion fix #230 to calculate size of char array in the third argument. If #230 will get delayed I'll change this test to just use simple types to not stumble upon that composite access issue.

[i-garrison]

• I get Invalid overload of 'std::endl' for the following example, don't know if this is a use case you want to cover:

Thanks, this turned out to be a bug in implementation - fixed and added a test for fold expression inside binary expression.

[ddscharfe]

LGTM.

As the logic you implemented is not trivial (at least to me), it might be good to add some additional tests. E.g. there is no test for the parser part which checks if there are more than one folds. I don't know if you are aware, but you can launch the tests as coverage and you will get a report about which code lines are covered by tests.
However there is no requirement for having a certain test coverage.

One thing I noticed is that the parser does not complain about some errors, e.g.

template<typename ... T>
void sum(T const ... vals) {
	auto foo = bar (...+vals);
}

or

template<typename ... T>
void sum(T const ... vals) {
	...+vals;
}

Not sure if this intended, but I think it is okay, because the compiler will complain later.

[i-garrison]

template<typename ... T>
void sum(T const ... vals) {
	...+vals;
}
    

Not sure if this intended, but I think it is okay, because the compiler will complain later.

I now fixed two cases that you indicated by limiting fold expression recognition to primary expression case. Will try to add more tests.

Looking back I might have started adding fold expression to parser in the wrong place (altering binary expression handling) but at the time changing the proper place (primary expression handling) looked too difficult for me.

[i-garrison]

Rebased to master after #230 merged to resolve ITypeMarshalBuffer constants.

[github-actions]

Test Results

     594 files       594 suites   17m 20s ⏱️
10 141 tests 10 118 ✔️ 22 💤 1 ❌
10 157 runs  10 134 ✔️ 22 💤 1 ❌

For more details on these failures, see this check.

Results for commit 44cab9a.

♻️ This comment has been updated with latest results.

[jonahgraham]

Looking back I might have started adding fold expression to parser in the wrong place (altering binary expression handling) but at the time changing the proper place (primary expression handling) looked too difficult for me.

I don't know if anyone else can comment/confirm that at the high level you have the addition in the correct place.

This change is also large and needs a quick run through the IP team, quoted from eclipsefdn/eca check:

Eclipse Intellectual Property Management:

Some contributions of code to be maintained by Eclipse Projects must be reviewed by the IP Team; in particular if they contain third party content and/or are over 1,000 Lines of Code. Further information can be read via the Eclipse Handbook.

[i-garrison]

This change is also large and needs a quick run through the IP team, quoted from eclipsefdn/eca check:

Eclipse Intellectual Property Management:
Some contributions of code to be maintained by Eclipse Projects must be reviewed by the IP Team; in particular if they contain third party content and/or are over 1,000 Lines of Code. Further information can be read via the Eclipse Handbook.

I'm sure this is mostly due to long lists of allowed fold expression operators )) Happy to assist if needed.

[jonahgraham]

The iplab entry is here https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6713

[jonahgraham]

This looks fine to me. Needs iplab resolved before it can be merged.

Thanks @i-garrison for the improvement and thanks @ddscharfe for reviewing.

[i-garrison]

The iplab entry is here https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6713

Hi @jonahgraham would you believe we can get this approved before next milestone deadline?

I'm thinking about yet another bugfix, which is not critical though - the fold expression validity check is currently too permissive. This does not affect valid code and does not affect parsing of fold expression either, so I'm planning to submit the improvement later after this PR is hopefully merged.

[jonahgraham]

The iplab entry is here https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6713

Hi @jonahgraham would you believe we can get this approved before next milestone deadline?

I hope so! My past experience of reviews of this type have been very quick. I have pinged the IP team too.

I'm thinking about yet another bugfix, which is not critical though - the fold expression validity check is currently too permissive. This does not affect valid code and does not affect parsing of fold expression either, so I'm planning to submit the improvement later after this PR is hopefully merged.

Fair enough, I hope that the IP process isn't too cumbersome. Once you have a track record of contributions then you can become a committer on the project, as a committer your changes don't need further IP reviews as the committer agreement handles that.

[jonahgraham]

The ipteam have completed their review and therefore I have merged this change.

Thanks @i-garrison!

[jonahgraham]

Oops, I guess I should have rebased the PR before merging it, unfortunately this PR did not apply cleanly, it led to compilation errors:

Error:  Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.5:compile (default-compile) on project org.eclipse.cdt.core.tests: Compilation failure: Compilation failure: 
Error:  /home/runner/work/cdt/cdt/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/cxx17/FoldExpressionTests.java:[108] 
Error:  	IASTTranslationUnit tu = parse(code, CPP, false, false);
Error:  	                         ^^^^^
Error:  The method parse(String, ParserLanguage, AST2TestBase.ScannerKind, boolean) in the type AST2TestBase is not applicable for the arguments (String, ParserLanguage, boolean, boolean)
Error:  /home/runner/work/cdt/cdt/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/cxx17/FoldExpressionTests.java:[120] 
Error:  	IASTTranslationUnit tu = parse(code, CPP, false, false);
Error:  	                         ^^^^^
Error:  The method parse(String, ParserLanguage, AST2TestBase.ScannerKind, boolean) in the type AST2TestBase is not applicable for the arguments (String, ParserLanguage, boolean, boolean)
Error: [ERROR] 2 problems (2 errors)
Error:  -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
Error:  
Error:  After correcting the problems, you can resume the build with the command
Error:    mvn <args> -rf :org.eclipse.cdt.core.tests
Error: Process completed with exit code 1.

I have created a revert PR here: #278 - but I will have a quick go fixing the error before I merge #278.

[jonahgraham]

#279 has the fixup. The issue was the change that had happened in parallel changing boolean -> ScannerKind in 5622e59