Bug 540373 - Format all CDT source code and enforce format standard
Summary: Format all CDT source code and enforce format standard
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-releng (show other bugs)
Version: 9.5.0   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 9.6.0   Edit
Assignee: Jonah Graham CLA
QA Contact: Doug Schaefer CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-22 10:08 EDT by Jonah Graham CLA
Modified: 2019-11-19 16:34 EST (History)
4 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jonah Graham CLA 2018-10-22 10:08:30 EDT 
At EclipseCon Embedded Summit we discussed formatting the entire code base to the Eclipse standard, with the exception of javadoc comments. As numerous Javadoc comments have a nice hand-crafted layout we don't want to lose  (for example, see org.eclipse.cdt.internal.core.dom.parser.cpp.GNUCPPSourceParser.postfixExpression(CastExprCtx, ITemplateIdStrategy))

In addition we will continue to enforce the standards by turning on Format on Save and storing the formatting in the projects.
Comment 1 Jonah Graham CLA 2018-10-22 10:34:35 EDT 
Announcement/request for feedback sent to cdt-dev: https://www.eclipse.org/lists/cdt-dev/msg33214.html

Target date is week of 12 Nov.
Comment 2 Marc-André Laperle CLA 2018-10-23 00:50:44 EDT 
Wishlist: - Remove trailing white space (Clean-up + save action).
Comment 3 Jonah Graham CLA 2018-10-23 15:20:13 EDT 
(In reply to Marc-André Laperle from comment #2)
> Wishlist: - Remove trailing white space (Clean-up + save action).

+1
Comment 4 Eclipse Genie CLA 2018-11-20 11:31:31 EST 
New Gerrit change created: https://git.eclipse.org/r/132779
Comment 5 Eclipse Genie CLA 2018-11-20 11:31:33 EST 
New Gerrit change created: https://git.eclipse.org/r/132778
Comment 6 Eclipse Genie CLA 2018-11-20 11:31:35 EST 
New Gerrit change created: https://git.eclipse.org/r/132777
Comment 7 Eclipse Genie CLA 2018-11-20 11:31:38 EST 
New Gerrit change created: https://git.eclipse.org/r/132780
Comment 8 Eclipse Genie CLA 2018-11-20 11:31:40 EST 
New Gerrit change created: https://git.eclipse.org/r/132781
Comment 9 Eclipse Genie CLA 2018-11-20 11:31:42 EST 
New Gerrit change created: https://git.eclipse.org/r/132782
Comment 10 Eclipse Genie CLA 2018-11-20 17:03:19 EST 
New Gerrit change created: https://git.eclipse.org/r/132795
Comment 11 Eclipse Genie CLA 2018-11-20 17:16:01 EST 
New Gerrit change created: https://git.eclipse.org/r/132797
Comment 12 Eclipse Genie CLA 2018-11-20 18:44:23 EST 
New Gerrit change created: https://git.eclipse.org/r/132802
Comment 13 Eclipse Genie CLA 2018-11-22 17:00:44 EST 
New Gerrit change created: https://git.eclipse.org/r/132920
Comment 14 Eclipse Genie CLA 2018-11-22 17:00:48 EST 
New Gerrit change created: https://git.eclipse.org/r/132923
Comment 15 Eclipse Genie CLA 2018-11-22 17:00:51 EST 
New Gerrit change created: https://git.eclipse.org/r/132921
Comment 16 Eclipse Genie CLA 2018-11-22 17:00:53 EST 
New Gerrit change created: https://git.eclipse.org/r/132925
Comment 17 Eclipse Genie CLA 2018-11-22 17:00:55 EST 
New Gerrit change created: https://git.eclipse.org/r/132922
Comment 18 Eclipse Genie CLA 2018-11-22 17:00:57 EST 
New Gerrit change created: https://git.eclipse.org/r/132924
Comment 19 Eclipse Genie CLA 2018-11-22 19:10:53 EST 
New Gerrit change created: https://git.eclipse.org/r/132930
Comment 20 Eclipse Genie CLA 2018-11-22 19:10:55 EST 
New Gerrit change created: https://git.eclipse.org/r/132929
Comment 21 Eclipse Genie CLA 2018-11-22 19:44:11 EST 
New Gerrit change created: https://git.eclipse.org/r/132931
Comment 22 Eclipse Genie CLA 2018-11-22 19:55:26 EST 
New Gerrit change created: https://git.eclipse.org/r/132932
Comment 23 Eclipse Genie CLA 2018-11-22 19:59:52 EST 
New Gerrit change created: https://git.eclipse.org/r/132933
Comment 41 Jonah Graham CLA 2018-11-23 07:05:58 EST 
All code submitted. Announced on CDT N&N and mailing list.

Only thing left is to see if we can enforce some of this on gerrits.
Comment 42 Eclipse Genie CLA 2018-11-23 09:00:42 EST 
New Gerrit change created: https://git.eclipse.org/r/132975
Comment 43 Eclipse Genie CLA 2018-11-23 09:00:45 EST 
New Gerrit change created: https://git.eclipse.org/r/132974
Comment 44 Eclipse Genie CLA 2018-11-23 09:00:47 EST 
New Gerrit change created: https://git.eclipse.org/r/132973
Comment 48 Jonah Graham CLA 2018-11-23 10:09:03 EST 
There is now a CI job to check code formatting and a few other rules. The job is https://ci.eclipse.org/cdt/job/cdt-verify-code-cleanliness and it runs https://git.eclipse.org/r/#/c/132975/4/releng/scripts/check_code_cleanliness.sh which has some rules in it.
Comment 49 Jonah Graham CLA 2018-11-24 03:28:22 EST 
Reopened because there is some code that is warning now that used to not warn due to change in compiler settings.

From Nate:

It looks like a lot of new warnings appeared in the CDT codebase.

A lot of them are in test code. For example, in test files like AST2CPPTests.java, every import from org.eclipse.cdt.internal.* packages is marked with a "discouraged access" warning.

The tests have always used internal APIs; there is no reason to warn about that.

There are also new warnings in non-test code, some of which seem bogus. For example, in ASTCompletionNode.java: "ASTCompletionNode implements non-API interface IASTCompletionNode". That make no sense - ASTCompletionNode is in the same package as IASTCompletionNode, and it is our canonical implementation.

Is this fallout from the recent cleanliness changes?
Comment 50 Eclipse Genie CLA 2018-11-24 05:22:59 EST 
New Gerrit change created: https://git.eclipse.org/r/133020
Comment 51 Eclipse Genie CLA 2018-11-24 05:23:02 EST 
New Gerrit change created: https://git.eclipse.org/r/133019
Comment 52 Jonah Graham CLA 2018-11-24 05:26:54 EST 
> For example, in ASTCompletionNode.java: "ASTCompletionNode implements non-API interface IASTCompletionNode". That make no sense - ASTCompletionNode is in the same package as IASTCompletionNode, and it is our canonical implementation.

Nate, I see this warning in revision 3cf0297769 (before any code cleanup). I think it should not be there too, but it seems to be a bug in PDE rather than an incorrect setting. I have tested it on various Eclipse's I have around and it shows up as a warning in Oxygen.3+ and not Neon.1-. I'll try and raise a bug against PDE. If you are only seeing it since the code cleanup change then it is probably a more intricate bug. 

https://wiki.eclipse.org/PDE/API_Tools/Restrictions says: "Restrictions are not applied in the same bundle where API is defined." so it should not be possible to have this warning ever on ASTCompletionNode.


> A lot of them are in test code. For example, in test files like AST2CPPTests.java, every import from org.eclipse.cdt.internal.* packages is marked with a "discouraged access" warning.

> The tests have always used internal APIs; there is no reason to warn about that.

I am working on better refinement of this now. I'll report back on that soon.
Comment 53 Eclipse Genie CLA 2018-11-24 06:20:44 EST 
New Gerrit change created: https://git.eclipse.org/r/133023
Comment 54 Eclipse Genie CLA 2018-11-24 06:33:31 EST 
New Gerrit change created: https://git.eclipse.org/r/133024
Comment 55 Jonah Graham CLA 2018-11-24 06:33:49 EST 
(In reply to Jonah Graham from comment #52)
> I am working on better refinement of this now. I'll report back on that soon.

See https://git.eclipse.org/r/#/c/133019/2/releng/scripts/check_code_cleanliness.sh

I have turned off warnings for a number of JDT/PDE settings. This brings the number of warnings in o.e.c.core.tests/ui.tests to similar numbers as before. There are a few additional warnings which I would rather fix that disable the warnings. See https://git.eclipse.org/r/#/c/133024/
Comment 60 Jonah Graham CLA 2018-11-24 09:02:33 EST 
Nate/others. I think the most recent set of patches resolve the outstanding issues with increased warnings. Please reopen if there are things that still don't seem right.
Comment 61 Eclipse Genie CLA 2018-11-24 12:45:54 EST 
New Gerrit change created: https://git.eclipse.org/r/133029
Comment 63 Nathan Ridge CLA 2018-11-24 13:53:32 EST 
(In reply to Jonah Graham from comment #52)
> > For example, in ASTCompletionNode.java: "ASTCompletionNode implements non-API interface IASTCompletionNode". That make no sense - ASTCompletionNode is in the same package as IASTCompletionNode, and it is our canonical implementation.
> 
> Nate, I see this warning in revision 3cf0297769 (before any code cleanup). I
> think it should not be there too, but it seems to be a bug in PDE rather
> than an incorrect setting. I have tested it on various Eclipse's I have
> around and it shows up as a warning in Oxygen.3+ and not Neon.1-. I'll try
> and raise a bug against PDE. If you are only seeing it since the code
> cleanup change then it is probably a more intricate bug. 
> 
> https://wiki.eclipse.org/PDE/API_Tools/Restrictions says: "Restrictions are
> not applied in the same bundle where API is defined." so it should not be
> possible to have this warning ever on ASTCompletionNode.

It's possible that this has always been around and I just noticed it now.

> > A lot of them are in test code. For example, in test files like AST2CPPTests.java, every import from org.eclipse.cdt.internal.* packages is marked with a "discouraged access" warning.
> 
> > The tests have always used internal APIs; there is no reason to warn about that.
> 
> I am working on better refinement of this now. I'll report back on that soon.

Thanks, this is looking much better!
Comment 64 Jonah Graham CLA 2018-11-24 17:57:03 EST 
(In reply to Eclipse Genie from comment #62)
> Gerrit change https://git.eclipse.org/r/133029 was merged to [master].
> Commit:
> http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/
> ?id=5b21097125aadd908b731c750b02913250263142

These instructions on how to rebase an old commit are also in a blog post  https://kichwacoders.com/2018/11/24/cdt-has-been-reformatted/
Comment 65 Jonah Graham CLA 2019-11-19 16:34:04 EST 
See Bug 553231 (for launchbar) and Bug 553233 (for tools.templates)