This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Use ; as separator for HeaderFileExtensions
ClosedPublic

Authored by jroelofs on Mar 4 2020, 9:00 AM.

Diff Detail

Event Timeline

jroelofs created this revision.Mar 4 2020, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 9:00 AM
jroelofs updated this revision to Diff 248206.Mar 4 2020, 9:08 AM

Fix comments

lebedev.ri requested changes to this revision.Mar 4 2020, 9:18 AM
lebedev.ri added a subscriber: lebedev.ri.

This will break existing .clang-tidy configs

This revision now requires changes to proceed.Mar 4 2020, 9:18 AM

The llvm::StringRef::split function can take multiple split characters, So it would be best to split on both for the time being but mark cases where , is used as deprecated. This way we keep the options in line with other clang-tidy checks but keep some backwards compatibility but in future change it to just using ;

jroelofs updated this revision to Diff 248245.Mar 4 2020, 10:44 AM

Preserve backwards compatibility of ',' as a delimiter (for now).

The llvm::StringRef::split function can take multiple split characters,

AFAIU, that's for multi-character delimiters, not multiple delimiters.

Preserve backwards compatibility of ',' as a delimiter (for now).

The llvm::StringRef::split function can take multiple split characters,

AFAIU, that's for multi-character delimiters, not multiple delimiters.

Yeah just checked it out my bad.

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
41

if (AllFileExtensions.contains(Delimeter)) {

jroelofs updated this revision to Diff 248357.Mar 4 2020, 4:46 PM
jroelofs marked an inline comment as done.
aaron.ballman accepted this revision.Mar 5 2020, 1:31 PM

LGTM with a minor nit.

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
40–46

Can drop the top-level const here.

FWIW i'm personally still unconvinced this is fixing a real problem

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
43–45

Can this spam be avoided?
It's going to be really intrusive, and it is not always possible to
just perform the migration (think: using more than one clang-tidy version.)
The deprecation message should be in the docs/releasenotes.

njames93 added inline comments.Mar 6 2020, 2:28 AM
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
43–45

That seems like a good idea. Would take a good many versions before the comma could safely be removed without fallout so people would get the warning too often and not be able to change it

aaron.ballman added inline comments.Mar 6 2020, 4:57 AM
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
43–45

FWIW, I was on the fence about this being a chatty diagnostic as well. I eventually figured that it was fine because it's going to stderr and it definitely gets the job done of letting people know. Most folks don't read release notes, so putting the information there is a good idea, but not really sufficient for warning about deprecation.

I don't think we have a better solution at hand and the point @lebedev.ri brings up about older versions is a valid one, so I agree, let's move this into the release notes rather than spamming the command line.

jroelofs updated this revision to Diff 248755.Mar 6 2020, 8:59 AM
  • Don't spam the deprecation message. Move that to release notes.
  • Drop unnecessary const.
jroelofs updated this revision to Diff 248757.Mar 6 2020, 9:03 AM
  • Drop dead include.
jroelofs marked 4 inline comments as done.Mar 6 2020, 9:04 AM
lebedev.ri resigned from this revision.Mar 9 2020, 6:16 AM

No further comments from me, thanks

This revision is now accepted and ready to land.Mar 9 2020, 6:16 AM
njames93 accepted this revision.Mar 9 2020, 6:28 AM

LGTM

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
44–47

Doesn't belong in this patch, but this function should be changed in a follow up to return an Optional<FileExtensionSet>.

jroelofs marked an inline comment as done.Mar 9 2020, 8:01 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
44–47

Looking at the uses, I'm not convinced this would be better in this specific case.

Compare:

if (Optional<FileExtensionSet> HFE = parseFileExtensions(Extensions, Delimiters)) {
  HeaderFileExtensions = *HFE;
} else {
  errs() << "Invalid extensions: " << Extensions;
}

With the status quo:

if (!parseFileExtensions(Extensions, HeaderFileExtensions, Delimiters)) {
  errs() << "Invalid extensions: " << Extensions;
}

Optional's explicit operator bool is great for when you want to guard on the presence of the thing, but not so great when you want to check for it not being there. I feel like we'd need some new utility to go with Optional to make this nice:

if (not(HeaderFileExtensions) = parseFileExtensions(Extensions, Delimiters)) {
  errs() << "Invalid extensions: " << Extensions;
}
njames93 added inline comments.Mar 9 2020, 8:25 AM
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
44–47

To be honest it should probably be Expected rather than Optional, but that still doesn't help solve the cleanliness issue.

jroelofs marked an inline comment as done.Mar 9 2020, 8:30 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
44–47

Implementation of the not idea here:

https://gcc.godbolt.org/z/XZh39g