Provide Standard C and C++ Language (without GNU extensions) #184

TheShermanTanker · 2022-11-29T14:43:50Z

WIP - Implement Standard C and C++ as language backends in CDT as an initial experimental step to pave the way for other implementations in the future (MSVC C, etc)

jonahgraham · 2022-11-29T15:05:29Z

Until you have had a commit merged into CDT I need to manually approve the checks so that they run. Please ping me if I haven't triggered them (or better yet, send me a simple PR I can merge :)

Can you update summary a little as even though this is a draft, it is still visible to everyone.

github-actions · 2022-11-29T16:34:30Z

Test Results

0 tests 0 ✔️ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit 7833735.

♻️ This comment has been updated with latest results.

TheShermanTanker · 2022-11-30T07:38:42Z

Can you update summary a little as even though this is a draft, it is still visible to everyone.

It's regarding the conversation we had in #178, specifically:

because no one has contributed a subclass of AbstractCLikeLanguage for MSVC

I think it's probably time that changed ;)

I am, however, trying to add Standard C and C++ in this patch first (without any compiler specific syntax), the reasoning being that Standard C/C++ is likely much easier to implement than a dialect of it, which makes adding it as a language implementation way more useful as a testbed to experiment on how CDT will react to such a change, and work out all the kinks accordingly. I'm fairly certain that once I get this right, doing the same for MSVC C and other forms of C will be a breeze

jonahgraham · 2023-01-01T18:55:11Z

@TheShermanTanker with #225 merged I think that workflows will run automatically for you now that you have a merged PR. If you need to do lots of build & tests in a draft mode I recommend turning on GitHub actions on your fork and add support to the list of branches to build in your fork:

cdt/.github/workflows/build-test.yml

Line 5 in c98aae4

branches: [ "main", "cdt_11_0" ]

TheShermanTanker · 2023-04-20T04:28:17Z

@jonahgraham I think this might be ready for review now, most of the changes are actually in the new parsers, the rest is just minor code refactoring to other parts of the IDE

jonahgraham

I have done a first pass review on this. This is a large change and getting it well reviewed is going to take significant effort. Lets start discussing the initial items I have raised before I do more reviewing.

There is a large section of copied (but modified) code e.g. GNUCPPSourceParser -> CPPSourceParser. I don't think we can support copying that code in the first place as maintaining such forks of that code going forward is unsustainable. And to be able to review it properly we would need to understand why make the changes that were made.

I'm seeing lots of API errors, here is the list in the IDE I see (note that there was another API error that I fixed in #375 so you'll need to rebase to pick them up).

Description	Resource	Path	Location	Type
Missing @since tag on org.eclipse.cdt.core.dom.ast.c.CLanguage	CLanguage.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/c	line 22	@since tag problem
Missing @since tag on org.eclipse.cdt.core.dom.ast.cpp.CPPLanguage	CPPLanguage.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp	line 22	@since tag problem
Missing @since tag on org.eclipse.cdt.core.dom.parser.cpp.CPPScannerExtensionConfiguration	CPPScannerExtensionConfiguration.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/parser/cpp	line 8	@since tag problem
Missing @since tag on org.eclipse.cdt.core.dom.parser.c.CScannerExtensionConfiguration	CScannerExtensionConfiguration.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/parser/c	line 8	@since tag problem
Invalid @since 5.11 tag on c__HAS_FEATURE; expecting @since 8.2	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 48	@since tag problem
Invalid @since 7.1 tag on c__HAS_INCLUDE_NEXT; expecting @since 8.2	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 53	@since tag problem
Missing @since tag on cASSERT	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 64	@since tag problem
Missing @since tag on cIDENT	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 62	@since tag problem
Missing @since tag on cIMPORT	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 61	@since tag problem
Missing @since tag on cINCLUDE_NEXT	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 60	@since tag problem
Missing @since tag on cpMAX	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 57	@since tag problem
Missing @since tag on cpMIN	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 56	@since tag problem
Missing @since tag on cSCCS	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 63	@since tag problem
Missing @since tag on cUNASSERT	GCCKeywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 65	@since tag problem
The field org.eclipse.cdt.core.dom.ast.IASTUnaryExpression.op_integerPack has been removed	IASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast	line 23	Compatibility Problem
The field org.eclipse.cdt.core.dom.ast.IASTUnaryExpression.op_labelReference has been removed	IASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast	line 23	Compatibility Problem
Missing @since tag on org.eclipse.cdt.core.dom.parser.ICFamilyLanguage	ICFamilyLanguage.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/parser	line 11	@since tag problem
Missing @since tag on org.eclipse.cdt.core.dom.parser.IExpressionParser	IExpressionParser.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/parser	line 7	@since tag problem
Invalid @since 5.8 tag on op_labelReference; expecting @since 8.2	IGNUASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/gnu	line 27	@since tag problem
Invalid @since 6.11 tag on op_integerPack; expecting @since 8.2	IGNUASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/gnu	line 33	@since tag problem
The field org.eclipse.cdt.core.dom.ast.gnu.IGNUASTUnaryExpression.op_integerPack in an interface that is intended to be implemented or extended has been added	IGNUASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/gnu	line 33	Compatibility Problem
The field org.eclipse.cdt.core.dom.ast.gnu.IGNUASTUnaryExpression.op_labelReference in an interface that is intended to be implemented or extended has been added	IGNUASTUnaryExpression.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/gnu	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.c__HAS_FEATURE has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.c__HAS_INCLUDE_NEXT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cASSERT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cIDENT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cIMPORT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cINCLUDE_NEXT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cpMAX has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cpMIN has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cSCCS has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The field org.eclipse.cdt.core.parser.Keywords.cUNASSERT has been removed	Keywords.java	/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/parser	line 27	Compatibility Problem
The major version should be incremented in version 8.2.0, since API breakage occurred since version 8.1.1	MANIFEST.MF	/org.eclipse.cdt.core/META-INF	line 5	Version Numbering Problem

Unfortunately you can't remove any API without following the process to remove it. (Moving it is equivalent to removing and adding new API.) This means either waiting two years (assuming that there is no objection) or having a major new version (which is unlikely until late 2024)

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp/CPPLanguage.java

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

jonahgraham · 2023-04-21T18:02:57Z

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/gnu/IGNUASTUnaryExpression.java

 * @noreference This interface is not intended to be referenced by clients.
 */
-@Deprecated


Removing the deprecation of this interface is unexpected. There is no concrete classes that implement this interface.

jonahgraham · 2023-04-21T18:08:07Z

...se.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/AbstractGNUSourceCodeParser.java

@@ -1109,7 +1114,7 @@ private IASTExpression buildExpression(IASTExpression left, BinaryOperator opera
 			break;
 		case IToken.tAND:
 			op = IASTBinaryExpression.op_logicalAnd;
-			unaryOp = IASTUnaryExpression.op_labelReference;
+			unaryOp = IGNUASTUnaryExpression.op_labelReference;


This comment goes with the comment about IGNUASTUnaryExpression not being deprecated anymore.

This leads to a strange case as unaryOp is passed to (eventually) the constructor of one of the concrete classes of IASTUnaryExpression (see call hierarchy of INodeFactory.newUnaryExpression(int, IASTExpression)

I suppose to complete the split there should be a new subclass of org.eclipse.cdt.internal.core.dom.parser.NodeFactory which is GNU vs non-GNU specific.

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

There is a large section of copied (but modified) code e.g. GNUCPPSourceParser -> CPPSourceParser. I don't think we can support copying that code in the first place as maintaining such forks of that code going forward is unsustainable. And to be able to review it properly we would need to understand why make the changes that were made.

Have a look at this PR to get a sense of why I don't think there should be two copies of such similar code.

TheShermanTanker · 2023-04-22T04:24:13Z

My apologies, I overlooked that API document for API changes (currently reading the first few bits of your review. I'll need some time to read through all of it).

TheShermanTanker · 2023-05-01T15:53:49Z

Hi @jonahgraham, sorry for such a long period of radio silence, I've been busy with my work on the JDK for a while. I'll start with the easier issues first: What should the copyright headers for the new source files be like? I can't be too sure since the headers in other files are all from companies or institutions, not individual people like myself, I apologize if this is a silly question

jonahgraham · 2023-05-01T15:59:17Z

Hi @jonahgraham, sorry for such a long period of radio silence, I've been busy with my work on the JDK for a while.

No worries - and thanks for coming back to this.

I'll start with the easier issues first: What should the copyright headers for the new source files be like? I can't be too sure since the headers in other files are all from companies or institutions, not individual people like myself, I apologize if this is a silly question

If you own the copyright of the code (which is what it sounds like) you should make it your name, e.g.

/*******************************************************************************
 * Copyright (c) 2023 Julian Waters.
 *
 * This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License 2.0
 * which accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 */

If the file is basically copied, make sure to copy the original copyright notice too.

…as possible

TheShermanTanker · 2023-05-18T09:07:06Z

Alright, I finally had time to do a first pass sweep of all the initial issues raised, hopefully this will fix as many of the API errors as possible

TheShermanTanker · 2023-05-29T06:49:30Z

@jonahgraham How's the API errors looking?

All the duplicated code is pretty much using the established code from the GNU Parsers but modified to remove all the GNU extensions from the parser, developing an entire new parser from scratch seemed like a significant effort when there's already code that can be re-adapted to fit the role of the new ISO language parser. I'm not sure how to approach this issue, unfortunately

jonahgraham · 2023-05-29T14:30:54Z

Sorry @TheShermanTanker I have fallen behind again (lost a week+ due to being sick :( ) - I will look again in the coming days.

TheShermanTanker · 2023-05-29T15:21:34Z

Sorry @TheShermanTanker I have fallen behind again (lost a week+ due to being sick :( ) - I will look again in the coming days.

Oh, no worries! Get well soon :)

jonahgraham · 2023-06-06T18:41:48Z

@jonahgraham How's the API errors looking?

The API errors are in better shape now. The adding of op_integerPack and op_labelReference to IGNUASTUnaryExpression is technically a breaking change, but I think we'll probably just add a filtered exception for those like this:

diff --git a/core/org.eclipse.cdt.core/.settings/.api_filters b/core/org.eclipse.cdt.core/.settings/.api_filters
index dda81c3a1b..bb58781e77 100644
--- a/core/org.eclipse.cdt.core/.settings/.api_filters
+++ b/core/org.eclipse.cdt.core/.settings/.api_filters
@@ -1,5 +1,19 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
 <component id="org.eclipse.cdt.core" version="2">
+    <resource path="parser/org/eclipse/cdt/core/dom/ast/gnu/IGNUASTUnaryExpression.java" type="org.eclipse.cdt.core.dom.ast.gnu.IGNUASTUnaryExpression">
+        <filter id="403767336">
+            <message_arguments>
+                <message_argument value="org.eclipse.cdt.core.dom.ast.gnu.IGNUASTUnaryExpression"/>
+                <message_argument value="op_integerPack"/>
+            </message_arguments>
+        </filter>
+        <filter id="403767336">
+            <message_arguments>
+                <message_argument value="org.eclipse.cdt.core.dom.ast.gnu.IGNUASTUnaryExpression"/>
+                <message_argument value="op_labelReference"/>
+            </message_arguments>
+        </filter>
+    </resource>
     <resource path="utils/org/eclipse/cdt/utils/debug/dwarf/DwarfConstants.java" type="org.eclipse.cdt.utils.debug.dwarf.DwarfConstants">
         <filter id="336658481">
             <message_arguments>
-- 
2.40.1

The old names that are left around that you have marked @Deprecated probably want forRemoval set to true. The @Deprecated items also need to have a comment that indicates what consumers of that API should use instead, e.g. on op_labelReference add a comment/link to the new location for this field.

"API removals and API breakages, actual or planned, should be added to New and Noteworthy's API Changelog" - quoted from https://github.com/eclipse-cdt/cdt/blob/main/POLICY.md#deprecating-and-deleting-api

All the duplicated code is pretty much using the established code from the GNU Parsers but modified to remove all the GNU extensions from the parser, developing an entire new parser from scratch seemed like a significant effort when there's already code that can be re-adapted to fit the role of the new ISO language parser. I'm not sure how to approach this issue, unfortunately

I don't want a newly written parser, I just don't think we should copy-paste nearly 3000 lines of code from AbstractGNUSourceCodeParser to AbstractCFamilySourceCodeParser. I imagined that AbstractGNUSourceCodeParser would extend AbstractCFamilySourceCodeParser? Same about CPPSourceParser being copied from GNUCPPSourceParser

I have an additional question - what is the plan for how to deal with things like the modifications made to CPreprocessor? The current PR doesn't error out on the keywords moved to GCCKeywords (AFAICT) when in non-GNU mode. Similar question applies to other places - EvalUnary's changes are another good example.

TheShermanTanker · 2023-06-07T06:38:17Z

Hi Jonah, thanks for getting back to me! :)

Hmm, I had not thought of the utilities like CPreprocessor - I was under the impression that changing the recognized keywords in the XSourceParser classes were enough to change the behaviour. I think we can probably subclass them for the GNU variant instead, and have the original only process C/C++ code without any extensions? Or at least that seems like the best approach to me

I don't want a newly written parser, I just don't think we should copy-paste nearly 3000 lines of code from AbstractGNUSourceCodeParser to AbstractCFamilySourceCodeParser. I imagined that AbstractGNUSourceCodeParser would extend AbstractCFamilySourceCodeParser? Same about CPPSourceParser being copied from GNUCPPSourceParser

Hmm, I can understand AbstractGNUSourceCodeParser extending CFamily one, but if GNUCPPSourceParser extended CPPSourceParser, wouldn't it no longer be an instance/subclass of the AbstractGNUSorceCodeParser?

In any case, I'll deal with the easier fixes first so I can make some progress haha

jonahgraham · 2023-06-07T19:07:20Z

but if GNUCPPSourceParser extended CPPSourceParser, wouldn't it no longer be an instance/subclass of the AbstractGNUSorceCodeParser?

Yes - the lack of multiple inheritance in Java may make this refactoring more challenging. Maybe adding additional interfaces would work. I haven't thought through what the full class diagram would look like, maybe it is worth drawing up what the class diagram looks like before refactoring too much? As my understanding is the original purpose of this PR is to make it possible to add an MSVC specialization in the future, including how extenders would fit in probably makes sense.

jonahgraham · 2023-08-18T17:15:25Z

I'm going to move this into draft until you are ready to return to this.

jonahgraham · 2023-10-11T16:58:30Z

I am cleaning up our backlog of open Pull Requests. Please re-open this PR if you plan to pick up this work again soon.

TheShermanTanker marked this pull request as draft November 29, 2022 14:43

jonahgraham changed the title ~~Draft~~ Provide Standard C and C++ Language (without GNU extensions) Nov 30, 2022

TheShermanTanker force-pushed the support branch from 5b5f2c9 to 4521027 Compare December 20, 2022 18:28

TheShermanTanker force-pushed the support branch from a47fd86 to fe72d3b Compare January 3, 2023 10:14

TheShermanTanker force-pushed the support branch from ab2b0a0 to 8771d78 Compare February 19, 2023 12:14

TheShermanTanker added 20 commits April 19, 2023 18:30

Start Support for Standard C and C++

094b6a7

Interface Identification

ebde219

Rearrange Keywords

2f38829

Move Clang extension out of Keywords

f3f6885

CScannerExtensionConfiguration and CPPScannerExtensionConfiguration

819d6fc

Comments

06049da

CSourceParser and CPPSourceParser

0cbef54

IActualCLanguage and IActualCPPLanguage should not be public

6da2fa6

Groundwork for CSourceParser and CPPSourceParser

63d1182

Move common logic to Abstract class

2699c25

Progress

eea44f4

Erroneous removal of org.osgi.framework.Version import

71520a6

Lay down foundations for the Parser

a5a71fd

Parser Subclasses

9444ec7

Names

4495d69

Progress

8ba82f5

Ditch K&R C

62e09df

Finish off C

f60b6ff

CPPSourceParser

6c44a16

Finish off C++ Parser and do finishing touches

f536a96

TheShermanTanker force-pushed the support branch from 7833735 to f536a96 Compare April 19, 2023 10:31

Build Errors

a9b873c

TheShermanTanker marked this pull request as ready for review April 19, 2023 10:51

TheShermanTanker added 3 commits April 19, 2023 21:13

Resolve Build Errors

19618d8

Missing Fold Expressions

cba97c2

New IExpressionParser

0da052d

jonahgraham requested changes Apr 21, 2023

View reviewed changes

Copyright, re-add moved API as Deprecated and fix as many API errors …

27a009c

…as possible

jonahgraham marked this pull request as draft August 18, 2023 17:15

jonahgraham closed this Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide Standard C and C++ Language (without GNU extensions) #184

Provide Standard C and C++ Language (without GNU extensions) #184

TheShermanTanker commented Nov 29, 2022 •

edited

Loading

jonahgraham commented Nov 29, 2022

github-actions bot commented Nov 29, 2022 •

edited

Loading

TheShermanTanker commented Nov 30, 2022

jonahgraham commented Jan 1, 2023

TheShermanTanker commented Apr 20, 2023

jonahgraham left a comment

jonahgraham Apr 21, 2023

jonahgraham Apr 21, 2023

jonahgraham commented Apr 21, 2023

TheShermanTanker commented Apr 22, 2023

TheShermanTanker commented May 1, 2023

jonahgraham commented May 1, 2023

TheShermanTanker commented May 18, 2023

TheShermanTanker commented May 29, 2023

jonahgraham commented May 29, 2023

TheShermanTanker commented May 29, 2023

jonahgraham commented Jun 6, 2023

TheShermanTanker commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Aug 18, 2023

jonahgraham commented Oct 11, 2023

Provide Standard C and C++ Language (without GNU extensions) #184

Provide Standard C and C++ Language (without GNU extensions) #184

Conversation

TheShermanTanker commented Nov 29, 2022 • edited Loading

jonahgraham commented Nov 29, 2022

github-actions bot commented Nov 29, 2022 • edited Loading

Test Results

TheShermanTanker commented Nov 30, 2022

jonahgraham commented Jan 1, 2023

TheShermanTanker commented Apr 20, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham Apr 21, 2023

Choose a reason for hiding this comment

jonahgraham Apr 21, 2023

Choose a reason for hiding this comment

jonahgraham commented Apr 21, 2023

TheShermanTanker commented Apr 22, 2023

TheShermanTanker commented May 1, 2023

jonahgraham commented May 1, 2023

TheShermanTanker commented May 18, 2023

TheShermanTanker commented May 29, 2023

jonahgraham commented May 29, 2023

TheShermanTanker commented May 29, 2023

jonahgraham commented Jun 6, 2023

TheShermanTanker commented Jun 7, 2023

jonahgraham commented Jun 7, 2023

jonahgraham commented Aug 18, 2023

jonahgraham commented Oct 11, 2023

TheShermanTanker commented Nov 29, 2022 •

edited

Loading

github-actions bot commented Nov 29, 2022 •

edited

Loading