Add basic support for c++17 deduction guides #344

i-garrison · 2023-03-27T22:46:17Z

No description provided.

jonahgraham · 2023-04-04T18:18:26Z

@ddscharfe are you interested in doing another review?

@i-garrison this one is a bit bigger and I think it needs me to have more knowledge before I can review it properly. I will revisit if @ddscharfe isn't able to review.

ddscharfe · 2023-04-06T10:52:21Z

@i-garrison @jonahgraham I'm afraid I won't be able to find the time to review this thoroughly very soon. Maybe I can do a quick review next week, but I can't promise.

jonahgraham

My overall impression is that this change looks of @i-garrison's standard high quality PRs. I don't quite follow all the changes as many of them are in areas I am not familiar with. I would really like it if someone at least gives it a once over and I am grateful @ddscharfe that you may have a few minutes for this.

jonahgraham · 2023-04-10T15:45:04Z

...ipse.cdt.core.tests/parser/org/eclipse/cdt/internal/index/tests/IndexDeductionGuideTest.java

+public abstract class IndexDeductionGuideTest extends IndexBindingResolutionTestBase {
+	private static void cxx17SetUp() {
+		//	// STD C++ parts
+		//	TestScannerProvider.sDefinedSymbols.put("__SIZEOF_SHORT__", "2");


Why is this commented out?

Thanks, I was not sure if it will have a lot of sense to somehow join AST2TestBase support for version-specific sets of macros but in the end only specific feature-detection was needed for new test. Cleaned this up now.

jonahgraham · 2023-04-10T22:43:20Z

PS: This branch has conflicts that must be resolved conflicting files:

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/internal/core/dom/parser/cpp/GNUCPPSourceParser.java

i-garrison · 2023-04-11T01:32:10Z

Rebased and fixed the conflict due to change in the same method.

ddscharfe · 2023-04-17T09:53:40Z

My overall impression is that this change looks of @i-garrison's standard high quality PRs.
+1, @i-garrison I really appreciate the work you put into this (and all your other PRs).

My comments so far:

I couldn't get this to work in my CDT debugging instance, I must be doing something wrong. If I put the code of the tests (which do pass) into my debugging instance I get errors. I even set CPPASTTranslationUnit::getEnableClassTemplateArgumentDeduction to always return true in the code to make sure I didn't mess something up with the macro, but still.
Is there a reason not to always enable the deduction feature? I'd like to avoid adding methods to the ICPPASTTranslationUnit interface.
Unfortunately I don't have more time to dig further into the code and do an appropriate code review. As expected, it all looks very reasonable but I cannot approve it ATM.

i-garrison · 2023-04-17T18:15:05Z

Hi @ddscharfe what is your compiler version? I'm testing with GCC 11.3

Yes I believe deduction guides can be enabled unconditionally, but would like to understand what's wrong with feature macro first.

ddscharfe · 2023-04-18T06:44:00Z

Hi @ddscharfe what is your compiler version? I'm testing with GCC 11.3

Hi! I'm using gcc 8.5.0 (default on Rocky Linux 8)

Yes I believe deduction guides can be enabled unconditionally, but would like to understand what's wrong with feature macro first.

I think it is not very intuitive from a usability standpoint. One could argue that they could be enabled by default and can be disabled via the macro. However I'd try to avoid the conditional statements in the parser code, but if there is a good reason to make the feature optional the way you implemented it is fine.

i-garrison · 2023-04-18T07:06:06Z

Hi @ddscharfe what is your compiler version? I'm testing with GCC 11.3

Hi! I'm using gcc 8.5.0 (default on Rocky Linux 8)

I think what happens is that according to https://gcc.gnu.org/projects/cxx-status.html#cxx17 GCC 8.5 does have lower value of feature detection macro which explains why deduction guides are not enabled. We probably can lower the expected value without losing functionality though I did not checked what's changed since 8.5

Yes I believe deduction guides can be enabled unconditionally, but would like to understand what's wrong with feature macro first.

I think it is not very intuitive from a usability standpoint. One could argue that they could be enabled by default and can be disabled via the macro. However I'd try to avoid the conditional statements in the parser code, but if there is a good reason to make the feature optional the way you implemented it is fine.

Parsing the construct takes extra lookup, so this arguably should be made optional since unless parser is able to resolve the name already, lookup has to be performed even if there are no deduction guides defined.

On the other hand if code does not use deduction guides then the only way to get into that extra lookup is via parser bug, otherwise name should be resolved as before this change.

I can drop these conditionals together with feature macro if that would be the consensus here. I did not really looked at c++20 and later changes to deduction guides to see if feature macro test should be preserved but it would not be hard to restore it later anyway.

jonahgraham · 2023-04-21T18:38:34Z

I can drop these conditionals together with feature macro if that would be the consensus here. I did not really looked at c++20 and later changes to deduction guides to see if feature macro test should be preserved but it would not be hard to restore it later anyway.

I don't have a strong opinion one way or another. For a long time the CDT parser has been as unconditional as possible, meaning that some code is accepted by CDT but then rejected by the compiler.

As @ddscharfe has 👍 the comment I think there is as much consensus as we'll get that we drop the conditionals.

Enabling C++17 deduction guides unconditionally causes one of the tests for template instantiation to fail because one of instantiations can be done via implicit deduction guide using default template arguments. Test case is covering issue https://bugs.eclipse.org/bugs/show_bug.cgi?id=207840 and remaining erroneous cases are not affected. Amend the test and comment about change since C++17.

i-garrison · 2023-04-22T07:11:45Z

Thanks for this discussion! I now added another change to make deduction guides unconditional (so this can be easily reverted again if needed.)

There is one extra change required for unit test covering https://bugs.eclipse.org/bugs/show_bug.cgi?id=207840 discussion. One of test cases there should be amended because instantiation is done via implicit deduction guide using default template argument.

jonahgraham

I have raised https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/7922 for IP signoff as this change is > 1000 LOC.

jonahgraham · 2023-04-25T18:15:45Z

I have raised https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/7922 for IP signoff as this change is > 1000 LOC.

Approved!

i-garrison force-pushed the pr/c++17-deduction-guides-basic branch from e4ecc2f to 98f5736 Compare March 27, 2023 22:56

jonahgraham added the language C/C++ Language Support label Apr 4, 2023

jonahgraham reviewed Apr 10, 2023

View reviewed changes

i-garrison added 2 commits April 11, 2023 04:28

Recognize C++17 deduction guides feature test macro and pass it via TU

9d11c57

Add basic support for c++17 deduction guides

dc9731d

i-garrison force-pushed the pr/c++17-deduction-guides-basic branch from 98f5736 to 4b99528 Compare April 11, 2023 01:31

Add tests for c++17 deduction guides

2864b34

i-garrison force-pushed the pr/c++17-deduction-guides-basic branch from 4b99528 to 2864b34 Compare April 11, 2023 01:37

i-garrison added 2 commits April 22, 2023 09:52

Make c++17 deduction guides unconditionally enabled

531d5b0

jonahgraham approved these changes Apr 22, 2023

View reviewed changes

jonahgraham added the ipteam PR is waiting on a review from Eclipse Foundation IP team label Apr 22, 2023

jonahgraham added this to the 11.2.0 milestone Apr 22, 2023

jonahgraham removed the ipteam PR is waiting on a review from Eclipse Foundation IP team label Apr 25, 2023

jonahgraham merged commit b965559 into eclipse-cdt:main Apr 25, 2023

i-garrison deleted the pr/c++17-deduction-guides-basic branch April 26, 2023 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for c++17 deduction guides #344

Add basic support for c++17 deduction guides #344

i-garrison commented Mar 27, 2023

jonahgraham commented Apr 4, 2023

ddscharfe commented Apr 6, 2023

jonahgraham left a comment

jonahgraham Apr 10, 2023

i-garrison Apr 11, 2023

jonahgraham commented Apr 10, 2023

i-garrison commented Apr 11, 2023

ddscharfe commented Apr 17, 2023

i-garrison commented Apr 17, 2023

ddscharfe commented Apr 18, 2023 •

edited by jonahgraham

Loading

i-garrison commented Apr 18, 2023 •

edited

Loading

jonahgraham commented Apr 21, 2023

i-garrison commented Apr 22, 2023

jonahgraham left a comment

jonahgraham commented Apr 25, 2023

Add basic support for c++17 deduction guides #344

Add basic support for c++17 deduction guides #344

Conversation

i-garrison commented Mar 27, 2023

jonahgraham commented Apr 4, 2023

ddscharfe commented Apr 6, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Apr 10, 2023

Choose a reason for hiding this comment

i-garrison Apr 11, 2023

Choose a reason for hiding this comment

jonahgraham commented Apr 10, 2023

i-garrison commented Apr 11, 2023

ddscharfe commented Apr 17, 2023

i-garrison commented Apr 17, 2023

ddscharfe commented Apr 18, 2023 • edited by jonahgraham Loading

i-garrison commented Apr 18, 2023 • edited Loading

jonahgraham commented Apr 21, 2023

i-garrison commented Apr 22, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Apr 25, 2023

ddscharfe commented Apr 18, 2023 •

edited by jonahgraham

Loading

i-garrison commented Apr 18, 2023 •

edited

Loading