Skip to content

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 1, 2021

This is a logical revert of eb45907

Based on recently merged #4941, C99 comments are now OK. I never found
any rationale or even written down coding style for excluding them in
the first place.

Signed-off-by: Marc Herbert marc.herbert@intel.com

This is a logical revert of eb45907

Based on recently merged thesofproject#4941, C99 comments are now OK. I never found
any rationale or even written down coding style for excluding them in
the first place.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@paulstelian97
Copy link
Collaborator

I don't see how that PR hints at C99 comments being OK. They're only used for SPDX.

I do agree that avoiding C99 comments isn't exactly the best thing, especially given that we don't have compilers that can't handle them I think. Holding off on reviewing for now (no reason to reject, just not sure if this is fully to be accepted)

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 2, 2021

I don't see how that PR hints at C99 comments being OK. They're only used for SPDX.

https://github.com/thesofproject/sof/pull/4941/files uses // for some entire, 7 lines-long file headers like src/math/base2log.c and a couple others. SPDX is just one line.

In other files, C99 comments are not used at all, not even for SPDX.

Anyway, 4941 is not that important. The question this PR is asking is: does everyone agree to allow C99 comments? I suspect they've already been used for ages.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 3, 2021

https://github.com/thesofproject/sof/pull/4941/files uses // for some entire, 7 lines-long file headers like src/math/base2log.c and a couple others. SPDX is just one line.

I think I got it: some people seem to assume SPDX magically includes and interprets the free form copyright notice following the SPDX-License-Identifier line. It does not, it's not magical. It takes a bit more effort to make copyright information known to SPDX, example from the https://spdx.github.io/spdx-spec/file-tags/ Annex (Informative)

SPDX-FileCopyrightText: 2019 Jane Doe <jane@example.com>
SPDX-FileCopyrightText: Copyright 2008-2010 John Smith
SPDX-FileCopyrightText: Copyright Example Company
SPDX-FileCopyrightText: Copyright contributors to the Foo project.

Off-topic sorry.

@lgirdwood
Copy link
Member

@marc-hb have you told this to skip CI ? cant see any skip request in the PR, but the CI has stalled ?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 3, 2021

I didn't ask to SKIP CI and I don't know why it stalled this time. It stalled after running checkpatch and this is a pure checkpatch configuration change which... does not even apply to sof-ci/jenkins as it always accepted C99 comments, see example in #4941.

sof-ci/jenkins has been running fine in other PRs so I didn't care.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 3, 2021

SOFCI TEST

EDIT: many unavailable devices in https://sof-ci.01.org/sofpr/PR5046/build11320/devicetest/ but everything else is green.

@gkbldcig
Copy link
Collaborator

gkbldcig commented Dec 4, 2021

Can one of the admins verify this patch?

@lgirdwood lgirdwood merged commit 60be65b into thesofproject:main Dec 6, 2021
@marc-hb marc-hb deleted the allowC99comments branch December 6, 2021 17:17
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 6, 2021

So no objection from anyone about C99 // comments in any SOF code review anymore? Speak now or forever hold your peace...

PS: while very similar, the Linux kernel has its own codestyle and policies not defined by SOF.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 21, 2021

just not sure if this is fully to be accepted

I thought it was, but apparently it's still not: #5110 (comment)

Need to practice my tea leaves reading?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 19, 2023

For even more fun: SPDX requires different comment style for .h files versus .c files, see 82220c8

@dbaluta
Copy link
Collaborator

dbaluta commented Jan 19, 2023

@marc-hb everyone knows that, the real question here is why 😨

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 19, 2023

Apparently because .h files are often enough included in linnker scripts or assembly. Please help review this minor SPDX-README.md clarification:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants