Add basic support for c++20 three-way comparison operator #219

i-garrison · 2022-12-11T19:05:55Z

This adds initial support for operator<=>, corresponding Lexer option is enabled if __cpp_impl_three_way_comparison feature-test macro is found.

Support is very rudimentary (e.g. return type deduction is not implemented, expression evaluation is not handled etc..)

github-actions · 2022-12-11T21:19:36Z

Test Results

    593 files     593 suites 16m 12s ⏱️
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 4569487.

♻️ This comment has been updated with latest results.

ruspl-afed · 2022-12-12T07:39:15Z

Hello @i-garrison,

Thank you for your interest to CDT project.

There are several points to be completed to make CI happy:

sign (electonically) ECA, see https://github.com/eclipse-cdt/cdt/blob/main/CONTRIBUTING.md#eclipse-contributor-agreement
Increment service segment of changed bundle by 100, i.e. set 8.0.100 here https://github.com/eclipse-cdt/cdt/blob/main/core/org.eclipse.cdt.core/META-INF/MANIFEST.MF#L5

jonahgraham · 2022-12-12T15:22:15Z

@i-garrison Thank you for the PR.

I put a callout on cdt-dev archives for some help reviewing this.

i-garrison · 2023-01-05T23:13:57Z

Hi @jonahgraham thanks for looking into this.
My main concern here would be the method used to enable recognition of three-way comparison operator based on feature macro __cpp_impl_three_way_comparison test. Is this allowed at all?

jonahgraham · 2023-01-16T16:48:33Z

Is this allowed at all?

I don't know. I have renewed my callout on cdt-dev and hope that someone will step forward. If not it sounds like I need to learn this area of the code as the original authors and subsequent maintainers are not active anymore.

ddscharfe

Hi @i-garrison,

I haven't fixed anything in the cdt parser yet, so please take my suggestions with a grain of salt :)
What I did to review your code was testing some use cases and searching in the cdt code for greaterEqual and in the tests for >= in order to find the places in the code which might have to be adapted.

My findings [1]:

The source code formatter is broken, <=> will be formatted to <= >
A case switch for op_threewaycomparison seems to be missing in org.eclipse.cdt.internal.core.dom.parser::VariableReadWriteFlags::rwInBinaryExpression
You might want to add a test case for the comparator to org.eclipse.cdt.core.parser.tests.ast2.AST2Tests::testCExpressions
You might want to add a test case for the comparator to /org.eclipse.cdt.core.tests/resources/rewrite/ASTWriterExpressionTestSource.awts
As you mentioned, the three way operator support is not complete, and you already added some // TODO tasks to the code. I found two places in the code, where I think you could add some more tasks:
org.eclipse.cdt.internal.core.dom.rewrite.astwriter::ExpressionWriter::getBinaryExpressionOperator
org.eclipse.cdt.internal.ui.refactoring.include::BindingClassifier::leave

[1] I didn't find a way to add comments to files which are not part of the commit, is this possible with github?

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/IASTBinaryExpression.java

.../org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/parser/scanner/CPreprocessor.java

ddscharfe · 2023-01-17T11:12:44Z

...core/parser/org/eclipse/cdt/internal/core/dom/parser/ASTAmbiguousBinaryVsCastExpression.java

 		case IASTBinaryExpression.op_shiftLeft:
 		case IASTBinaryExpression.op_shiftRight:
-			return 8;


I struggled a little bit to understand the use case of the binary vs cast ambiguation, as I couldn't come up with any examples to test this. However your changes look reasonable, you just added <=> and gave it a higher precedence.

i-garrison · 2023-01-17T18:28:38Z

The source code formatter is broken, <=> will be formatted to <= >

Thanks, I completely missed formatter changes here - will fix that.

i-garrison · 2023-01-19T20:59:20Z

I didn't find a way to add comments to files which are not part of the commit, is this possible with github?

Not possible, but there is a discussion about this feature community/community#9099

i-garrison · 2023-01-19T21:08:33Z

A case switch for op_threewaycomparison seems to be missing in org.eclipse.cdt.internal.core.dom.parser::VariableReadWriteFlags::rwInBinaryExpression

Fixed and added new test case to VariableReadWriteFlagsTest - unfortunately by default expression is READ so I tested that in debugger too ))

i-garrison · 2023-01-19T21:17:57Z

You might want to add a test case for the comparator to org.eclipse.cdt.core.parser.tests.ast2.AST2Tests::testCExpressions

This requires parsing via C++20-enabled scanner which would recognize new <=> token so I implemented new test case testCPP20Expressions instead. Unfortunately the supporting test code change that I was able to prepare looks a bit massive.

It might be easier to look at second and following commits individually, as test case changes for <=> themselves are rather compact.

i-garrison · 2023-01-19T21:18:49Z

You might want to add a test case for the comparator to /org.eclipse.cdt.core.tests/resources/rewrite/ASTWriterExpressionTestSource.awts

Implemented this and also added support for C++20 syntax hint in awts.

Add STD and GNU scanner configuration ids and pass these as needed instead of boolean flag useGNUExtensions. This will be used later by C++20 scanner tests.

i-garrison · 2023-01-19T21:31:32Z

Thank you @ddscharfe I fixed a couple of issues while doing tests (updated first commit with required changes.)

The source code formatter is broken, <=> will be formatted to <= >

This needs a bit of research - need to pass the C++20 with three-way-compare flag of code being formatted to formatter implementation but I do not see any flags being passed at all yet.

As you mentioned, the three way operator support is not complete, and you already added some // TODO tasks to the code. I found two places in the code, where I think you could add some more tasks:
org.eclipse.cdt.internal.core.dom.rewrite.astwriter::ExpressionWriter::getBinaryExpressionOperator

This is now implemented to fix awst rewriter test.

org.eclipse.cdt.internal.ui.refactoring.include::BindingClassifier::leave

I just left a shortcut for now in BingingClassifier - was not able to trigger that case yet.

ddscharfe · 2023-01-20T11:05:49Z

This needs a bit of research - need to pass the C++20 with three-way-compare flag of code being formatted to formatter implementation but I do not see any flags being passed at all yet.

You could use the fTextEditor in org.eclipse.cdt.ui.text.CSourceViewerConfiguration::getContentFormatter to get the macro definitions, something like this:

	@Override
	public IContentFormatter getContentFormatter(ISourceViewer sourceViewer) {
		final MultiPassContentFormatter formatter = new MultiPassContentFormatter(
				getConfiguredDocumentPartitioning(sourceViewer), IDocument.DEFAULT_CONTENT_TYPE);

		Supplier<Boolean> supportThreeway = () -> {
			try {
				if (fTextEditor instanceof ITranslationUnitHolder tuHolder) {
					IASTTranslationUnit ast = ((CEditor) fTextEditor).getTranslationUnit().getAST();
					if (ast != null) {
						IASTPreprocessorMacroDefinition[] builtinMacroDefinitions = ast.getBuiltinMacroDefinitions();
						// search for the threeway builtin macro
						return true;
					}
				}
			} catch (CoreException e) {
				e.printStackTrace();
			}
			return false;
		};
		
		formatter.setMasterStrategy(new CFormattingStrategy(supportThreeway));
		return formatter;
	}

However this brings me back your initial concern:

My main concern here would be the method used to enable recognition of three-way comparison operator based on feature macro __cpp_impl_three_way_comparison test. Is this allowed at all?

As far as I understand how CDT handled new language features until now (e.g. lambdas) was adding support in the parser, regardless of what compiler is used for building. So for instance when you have an old compiler which doesn't support lambdas, the C++ Editor will not mark lambdas as an error, unless you compile the project.
If my understanding is correct, I think you could remove the check, so the <=> operator support is always enabled. However, I might be totally wrong about this.

i-garrison · 2023-01-20T21:20:15Z

As far as I understand how CDT handled new language features until now (e.g. lambdas) was adding support in the parser, regardless of what compiler is used for building. So for instance when you have an old compiler which doesn't support lambdas, the C++ Editor will not mark lambdas as an error, unless you compile the project.
If my understanding is correct, I think you could remove the check, so the <=> operator support is always enabled.

To my mind this strategy will not work with new <=> token since there is a possibility that pre-c++20 code will now be invalid. One of example where using declaration turns into an error with c++20:

class A;
using Op = bool (&)(const A&, const A&);
template<Op op> class B;
bool operator<=(const A&, const A&);
using C = B<operator<=>;

I have never encountered this in the wild though, so the example could be a bit synthetic.

ddscharfe · 2023-01-21T14:45:03Z

To my mind this strategy will not work with new <=> token since there is a possibility that pre-c++20 code will now be invalid. One of example where using declaration turns into an error with c++20:
class A;
using Op = bool (&)(const A&, const A&);
template<Op op> class B;
bool operator<=(const A&, const A&);
using C = B<operator<=>;

Good point, and I'm sure having compilers pre-c++20 will remain a use case for quiet some time, so this would be a problem indeed. I still think that not having a flag/setting might be the best solution, as it avoids introducing new complexity (plus the user has to know how to enable this). But maybe there already is some possibility for such a setting which we're not aware of.

o.e.cdt.core 8.0.0 was released as part of CDT 11, so this new non-breaking API needs a version bump to 8.1.0.

jonahgraham

This looks promising so far. Not sure what it will take to see this to completion still, but with the feature flag we can probably merge sooner rather than later on it.

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/ValueFactory.java

jonahgraham · 2023-01-25T20:04:40Z

...org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/GNUCSourceParser.java

@@ -548,6 +548,7 @@ private IASTExpression expression(final ExprKind kind) throws EndOfFileException
 				break;
 			case IToken.tEQUAL:
 			case IToken.tNOTEQUAL:
+			case IToken.tTHREEWAYCOMPARISON:


In the C++ the <=> has different precedence than = and !=, but in C it has the same.

Is <=> part of C? I couldn't find reference to that in future C specs.

Operator <=> is C++ only because it's result is defined to be an object of ordering type defined in <compare>

but in C it has the same

This is a bug, should not be in C parser, dropped now.

.../org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/parser/scanner/CPreprocessor.java

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2TestBase.java

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser/Keywords.java

jonahgraham · 2023-01-25T20:43:12Z

...eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CASTBinaryExpression.java

@@ -234,6 +234,10 @@ public IType getExpressionType() {
 		case op_notequals:
 			return new CBasicType(Kind.eInt, 0, this);

+		case op_threewaycomparison:
+			// TODO: implement for <=>


Does this need implementing support for strong and partial ordering? Or can this be folded into the above cases as a return new CBasicType(Kind.eInt, 0, this);?

I did not looked into this, complete implementation should somehow inject types from <compare> header. This could be looked into later.

jonahgraham · 2023-01-25T20:46:04Z

...cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/BuiltinOperators.java

@@ -167,6 +167,10 @@ private ICPPFunction[] create() {
 			comparison(true);
 			break;

+		case THREEWAYCOMPARISON:
+			// TODO: implement for <=>


I have run out of time on this first pass to understand what this bit of code does and what needs to be done.

Planned to look into this later, together with synthesized comparison operators.

jonahgraham · 2023-01-25T20:47:08Z

...lipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalBinary.java

@@ -388,6 +389,10 @@ public IType computeType() {
 		case op_notequals:
 			return CPPBasicType.BOOLEAN;

+		case op_threewaycomparison:
+			// TODO: implement for <=>
+			return ProblemType.UNKNOWN_FOR_EXPRESSION;


Similar to other comment, does this todo require strong/partial ordering understanding, or can this just be a basic integer?

I planned to look into evaluation stuff later, this needs more investigation.

jonahgraham · 2023-01-26T18:48:19Z

For the remaining items, is this something that needs to be investigated/done before we merge, or as follow-up tasks later? If its the former, please move this to a draft. If it is the latter I will try to complete the review with that goal in mind.

i-garrison · 2023-01-26T18:53:58Z

For the remaining items, is this something that needs to be investigated/done before we merge, or as follow-up tasks later? If its the former, please move this to a draft. If it is the latter I will try to complete the review with that goal in mind.

I believe remaining items can be done later. This change should already be useful since it fixes parse errors with c++20 code because standard library now uses <=> left and right, and implementing more features of <=> can be done more or less incrementally as we go.

jonahgraham · 2023-01-26T19:20:58Z

@ddscharfe I would appreciate it if you could complete a final review before I merge this.

@i-garrison I am happy to merge as is so that we can get this in 2023-03 M2 on Monday and get it out there so that we can get feedback sooner. I'm just going to wait for @ddscharfe's review in case he has anything to add before it goes in.

i-garrison · 2023-01-26T19:48:01Z

Thanks for looking into this!
I see the diff is more than 1000 lines too, but do not see it marked for ip review yet. Most of the changes are in tests api though, is the tool marking this for ip review that smart?

jonahgraham · 2023-01-26T19:50:31Z

Thanks for looking into this! I see the diff is more than 1000 lines too, but do not see it marked for ip review yet. Most of the changes are in tests api though, is the tool marking this for ip review that smart?

My count was the diff was just < 1000 lines. Either way, there is some discretion involved here. As most of the lines of diff are in e1d429f which was changing the false/true to the enum, no need for review as that change is clearly not an IP concern.

ddscharfe

My count was the diff was just < 1000 lines. Either way, there is some discretion involved here. As most of the lines of diff are in e1d429f which was changing the false/true to the enum, no need for review as that change is clearly not an IP concern.

+1

jonahgraham · 2023-01-28T15:52:36Z

Thanks @i-garrison for improving user's experience with C++20. Thanks @ddscharfe for the reviews.

i-garrison marked this pull request as draft December 11, 2022 19:38

i-garrison marked this pull request as ready for review December 11, 2022 19:39

i-garrison force-pushed the pr/c++20-three-way-comparison-operator-basic branch from 8483ae5 to f871e85 Compare December 12, 2022 16:46

jonahgraham added the language C/C++ Language Support label Jan 16, 2023

ddscharfe reviewed Jan 17, 2023

View reviewed changes

i-garrison marked this pull request as draft January 17, 2023 18:28

i-garrison force-pushed the pr/c++20-three-way-comparison-operator-basic branch from f871e85 to 999137a Compare January 19, 2023 19:52

i-garrison force-pushed the pr/c++20-three-way-comparison-operator-basic branch from 999137a to bf0d0cc Compare January 19, 2023 21:01

i-garrison added 5 commits January 20, 2023 00:28

Add basic support for c++20 three-way comparison operator

39db0d6

Replace useGNUExtensions flag in test code with scanner configuration id

e1d429f

Add STD and GNU scanner configuration ids and pass these as needed instead of boolean flag useGNUExtensions. This will be used later by C++20 scanner tests.

Add test for c++20 three-way comparison expression

e8ff910

Add rewriter test for c++20 three-way comparison expression

dd1b34b

Add variable read/write flag test for c++20 three-way comparison

4223f05

i-garrison force-pushed the pr/c++20-three-way-comparison-operator-basic branch from 8c26177 to 4223f05 Compare January 19, 2023 21:31

i-garrison marked this pull request as ready for review January 19, 2023 21:33

Add operator name test for c++20 three-way comparison

96bb398

Fix precedence for c++20 three-way comparison operator

d3c309e

Add and correct since tags for new API in 8.1.0

f4b3ddd

o.e.cdt.core 8.0.0 was released as part of CDT 11, so this new non-breaking API needs a version bump to 8.1.0.

jonahgraham reviewed Jan 25, 2023

View reviewed changes

i-garrison added 2 commits January 26, 2023 11:24

Add simple evaluation for c++20 three-way comparison

aeba5f4

Drop three-way comparison from C parser, this is C++ only.

4569487

jonahgraham approved these changes Jan 26, 2023

View reviewed changes

jonahgraham requested a review from ddscharfe January 26, 2023 19:19

ddscharfe approved these changes Jan 27, 2023

View reviewed changes

jonahgraham merged commit 2776d17 into eclipse-cdt:main Jan 28, 2023

jonahgraham added this to the 11.1.0 milestone Jan 28, 2023

jonahgraham added the noteworthy Pull requests and fixed issues that should be highlighted to users label Jan 28, 2023

i-garrison deleted the pr/c++20-three-way-comparison-operator-basic branch January 28, 2023 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for c++20 three-way comparison operator #219

Add basic support for c++20 three-way comparison operator #219

i-garrison commented Dec 11, 2022

github-actions bot commented Dec 11, 2022 •

edited

Loading

ruspl-afed commented Dec 12, 2022

jonahgraham commented Dec 12, 2022

i-garrison commented Jan 5, 2023

jonahgraham commented Jan 16, 2023

ddscharfe left a comment

ddscharfe Jan 17, 2023

i-garrison commented Jan 17, 2023

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023 •

edited

Loading

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023

ddscharfe commented Jan 20, 2023 •

edited

Loading

i-garrison commented Jan 20, 2023

ddscharfe commented Jan 21, 2023

jonahgraham left a comment

jonahgraham Jan 25, 2023

i-garrison Jan 26, 2023

i-garrison Jan 26, 2023

jonahgraham Jan 25, 2023

i-garrison Jan 26, 2023

jonahgraham Jan 25, 2023

i-garrison Jan 26, 2023

jonahgraham Jan 25, 2023

i-garrison Jan 26, 2023

jonahgraham commented Jan 26, 2023

i-garrison commented Jan 26, 2023

jonahgraham commented Jan 26, 2023

i-garrison commented Jan 26, 2023

jonahgraham commented Jan 26, 2023

ddscharfe left a comment

jonahgraham commented Jan 28, 2023

Add basic support for c++20 three-way comparison operator #219

Add basic support for c++20 three-way comparison operator #219

Conversation

i-garrison commented Dec 11, 2022

github-actions bot commented Dec 11, 2022 • edited Loading

Test Results

ruspl-afed commented Dec 12, 2022

jonahgraham commented Dec 12, 2022

i-garrison commented Jan 5, 2023

jonahgraham commented Jan 16, 2023

ddscharfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-garrison commented Jan 17, 2023

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023 • edited Loading

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023

i-garrison commented Jan 19, 2023

ddscharfe commented Jan 20, 2023 • edited Loading

i-garrison commented Jan 20, 2023

ddscharfe commented Jan 21, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Jan 26, 2023

i-garrison commented Jan 26, 2023

jonahgraham commented Jan 26, 2023

i-garrison commented Jan 26, 2023

jonahgraham commented Jan 26, 2023

ddscharfe left a comment

Choose a reason for hiding this comment

jonahgraham commented Jan 28, 2023

github-actions bot commented Dec 11, 2022 •

edited

Loading

i-garrison commented Jan 19, 2023 •

edited

Loading

ddscharfe commented Jan 20, 2023 •

edited

Loading