Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cling] Remove support for declarations without 'auto' keyword #16410

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Sep 11, 2024

This Pull request:

Removes the implicit auto keyword support added in: 071d084 and 1f4a6dc

Changes or fixes:

This feature was deprecated in: #14645

Will update LLVM and squash the commits here before the final merge/decision.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

Copy link

github-actions bot commented Sep 11, 2024

Test Results

    13 files      13 suites   3d 1h 26m 43s ⏱️
 3 029 tests  3 027 ✅ 0 💤 2 ❌
33 856 runs  33 854 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 7a46308.

♻️ This comment has been updated with latest results.

@dpiparo dpiparo marked this pull request as ready for review September 12, 2024 06:17
Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm: thanks.

@hahnjo hahnjo self-requested a review September 12, 2024 06:58
Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we can do this right now: This is a big departure from CINT times and there are probably still tons of macros out there that rely on it. Initially we discussed only removing this for ROOT 7, did this decision change?

If we remove this, I think we can also get rid of a patch in Clang: root-project/llvm-project@fecc97d

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take this with a grain of salt as I remember how much efforts I spent to turn the compiler infrastructure upside down to support this feature...

This is awesome. LGTM!

I'd still wait for the input of @pcanal.

@dpiparo
Copy link
Member

dpiparo commented Sep 12, 2024

This was deprecated in 6.32, i.e.
image

@hahnjo
Copy link
Member

hahnjo commented Sep 12, 2024

This was deprecated in 6.32, i.e. image

Yes, but I don't think people will do every upgrade and especially not for old macros. This was discussed in the ROOT meeting back then when we agreed that it would only be removed for ROOT 7. I'm ok with revisiting this decision, but I'm against just merging a PR less than half a year after it was deprecated.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@guitargeek
Copy link
Contributor

Why was this deprecation not mentioned in the 6.32 release notes? https://root.cern/doc/v632/release-notes.html

The standard procedure is: if you deprecate something, write it in the release notes, including the information on when it will be eventually removed (which is usually one release later). If we don't follow our own standard here, we'll have these discussions forever :(

@hahnjo
Copy link
Member

hahnjo commented Sep 12, 2024

including the information on when it will be eventually removed (which is usually one release later)

In this case it was discussed: It will be removed in ROOT 7.

@guitargeek
Copy link
Contributor

Is that discussion documented somewhere?

@hahnjo
Copy link
Member

hahnjo commented Sep 12, 2024

Is that discussion documented somewhere?

Yes, in https://indico.cern.ch/event/1381548/

Regarding the deprecation auto auto: will break a lot of unnamed old macros, we should wait with removing until ROOT 7. We will start issuing deprecation messages starting from ROOT 6.32.

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Jonas that it is too soon. (Anecdotally, my fingers still rely on this feature when typing at the prompt and grumble at the 'spurrious' warning :) ).

@dpiparo
Copy link
Member

dpiparo commented Sep 26, 2024

I would like to link here this note #15368 (comment) by @devajithvs : once this is merged, PR #15368 will have to be reverted.

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.

6 participants