Page MenuHomePhabricator

[clang-tidy] minor improvements in modernise-deprecated-headers check
ClosedPublic

Authored by omtcyf0 on Mar 9 2016, 5:26 AM.

Details

Summary

This patch introduces a minor list of changes as proposed by Richard Smith in the mailing list.

See original comments with an impact on the future check state below:

[comments.begin

+ {"complex.h", "ccomplex"},

It'd be better to convert this one to <complex>, or leave it alone.
<ccomplex> is an unnecessary wart.

(The contents of C++11's <complex.h> / <ccomplex> / <complex> (all of
which are identical) aren't comparable to C99's <complex.h>, so if
this was C++98 code using the C99 header, the code will be broken with
or without this transformation.)

+ {"iso646.h", "ciso646"},

Just delete #includes of this one. <ciso646> does nothing.

+ {"stdalign.h", "cstdalign"},
+ {"stdbool.h", "cstdbool"},

We should just delete these two includes. These headers do nothing in C++.

comments.end]

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyf0 updated this revision to Diff 50125.Mar 9 2016, 5:26 AM
omtcyf0 retitled this revision from to [clang-tidy] minor improvements in modernise-deprecated-headers check.
omtcyf0 updated this object.
omtcyf0 added a subscriber: cfe-commits.
alexfh edited edge metadata.Mar 10 2016, 11:02 PM

+ {"iso646.h", "ciso646"},
Just delete #includes of this one. <ciso646> does nothing.

+ {"stdalign.h", "cstdalign"},
+ {"stdbool.h", "cstdbool"},
We should just delete these two includes. These headers do nothing in C++.

These don't seem to be addressed. Can you at least leave a FIXME in the code and in the test?

@alexfh, why? I've deleted these headers both from sources and from testset, didn't I?

@alexfh, why? I've deleted these headers both from sources and from testset, didn't I?

I think, you misunderstood Richard's comment. The check should remove includes of these three headers from the code, since they have no effect in C++.

alexfh added inline comments.Mar 18 2016, 3:52 AM
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
59 ↗(On Diff #50125)

According to http://www.cplusplus.com/reference/cinttypes/, this header should be changed in C++11 mode only. See also http://llvm.org/PR26982

67 ↗(On Diff #50125)

According to http://www.cplusplus.com/reference/cstdint/, this header is a part of C++11 standard, so we should move this to the list below.

docs/clang-tidy/checks/modernize-deprecated-headers.rst
23 ↗(On Diff #50125)

Is this header actually "deprecated since C++11"?

39 ↗(On Diff #50125)

Not sure if "deprecated" is the right word here. The wording seems to be slightly different: "This header simply includes <cmath> and <ccomplex>."

41 ↗(On Diff #50125)

It's not exactly "deprecated". It has appeared in C++11 and is not particularly useful (just contains a couple of feature macros). Maybe we should just remove #includes of this header as well.

alexfh requested changes to this revision.Mar 24 2016, 7:42 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 24 2016, 7:42 AM
omtcyf0 updated this revision to Diff 62701.EditedJul 4 2016, 11:43 AM
omtcyf0 edited edge metadata.

Hope it looks better now.

omtcyf0 marked 2 inline comments as done.Jul 4 2016, 11:44 AM
omtcyf0 added inline comments.
docs/clang-tidy/checks/modernize-deprecated-headers.rst
45 ↗(On Diff #62701)

At least that's what cppreference tells me.

alexfh added inline comments.Jul 4 2016, 11:30 PM
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
99 ↗(On Diff #62701)

nit: "occurrence"

docs/clang-tidy/checks/modernize-deprecated-headers.rst
45 ↗(On Diff #62701)

These checks don't have effect in C++:

Did you mean "headers"?

test/clang-tidy/modernize-deprecated-headers-cxx03.cpp
34 ↗(On Diff #62701)

How about placing the check lines right after the corresponding includes? This way the test will be easier to read and won't require careful line offsets in the check lines.

omtcyf0 updated this revision to Diff 62850.Jul 6 2016, 5:42 AM
omtcyf0 edited edge metadata.
omtcyf0 marked 3 inline comments as done.

resolved small problems

alexfh added inline comments.Jul 6 2016, 6:27 AM
docs/clang-tidy/checks/modernize-deprecated-headers.rst
39 ↗(On Diff #50125)

What about this comment? (was on the' <tgmath.h> -> <ctgmath>` line)

41 ↗(On Diff #50125)

What about this comment? (was on the' <uchar.h> -> <cuchar>` line)

@alexfh same applies to the other headers. All of them are marked as "deprecated" there.

Not sure if I should also mark <stdalign.h> and <stdbool.h> since they are in "has no effect in C++" section now, though.

Please update the tests according to the comment (https://reviews.llvm.org/D17990?id=62701#inline-186216).

docs/clang-tidy/checks/modernize-deprecated-headers.rst
43 ↗(On Diff #62850)

nit: "the previous list."

alexfh requested changes to this revision.Jul 19 2016, 7:31 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
39 ↗(On Diff #62850)

nit: DeleteHeaders

39 ↗(On Diff #62850)

Use llvm::StringSet

This revision now requires changes to proceed.Jul 19 2016, 7:31 AM
omtcyf0 updated this revision to Diff 67328.Aug 9 2016, 6:32 AM
omtcyf0 edited edge metadata.

Address comments Alex made.

omtcyf0 updated this revision to Diff 67334.Aug 9 2016, 7:03 AM
omtcyf0 edited edge metadata.
omtcyf0 marked 3 inline comments as done.

Address remaining comments.

alexfh requested changes to this revision.Aug 9 2016, 8:42 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
55 ↗(On Diff #67334)

It might not work on MSVC2013. You can try submitting this, but please watch the buildbots closely and have an alternative patch ready to fix the issue.

85 ↗(On Diff #67334)

ditto

test/clang-tidy/modernize-deprecated-headers-cxx03.cpp
5 ↗(On Diff #67334)

Please anchor the patterns to the start and end of line.

60 ↗(On Diff #67334)

Please add a comment after the #include and add a CHECK-FIXES to ensure the whole #include directive is removed.

131 ↗(On Diff #67334)

Add proper CHECK-FIXES here as well.

The same for the other test file.

This revision now requires changes to proceed.Aug 9 2016, 8:42 AM
omtcyf0 updated this revision to Diff 67488.Aug 10 2016, 2:18 AM
omtcyf0 edited edge metadata.
omtcyf0 marked 2 inline comments as done.

Address comments.

omtcyf0 marked 3 inline comments as done.Aug 10 2016, 2:19 AM
alexfh accepted this revision.Aug 10 2016, 4:54 AM
alexfh edited edge metadata.

LG

This revision is now accepted and ready to land.Aug 10 2016, 4:54 AM
aaron.ballman requested changes to this revision.Aug 10 2016, 5:20 AM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
79 ↗(On Diff #67488)

This will definitely fail in MSVC 2013.

89 ↗(On Diff #67488)

This one will work fine, but this pattern does not seem safe to me (same as above). StringSet::insert expects a StringRef. This vector (and all the std::string elements) will be destroyed at the end of the for loop, which means the StringSet will have dangling references.

This revision now requires changes to proceed.Aug 10 2016, 5:20 AM
omtcyfz added inline comments.
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
79 ↗(On Diff #67488)

See the left side of diff. It already is in source tree, buildbots never complained.

Thus, it won't.

docs/clang-tidy/checks/modernize-deprecated-headers.rst
39 ↗(On Diff #50125)
aaron.ballman added inline comments.Aug 10 2016, 7:18 AM
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
79 ↗(On Diff #67488)

Ah shoot, I was misremembering a different case with a similar construct, except it was using std::make_pair, which caused deduction to fail. Le sigh.

omtcyf0 updated this revision to Diff 67519.Aug 10 2016, 7:24 AM
omtcyf0 edited edge metadata.

Address comments.

omtcyf0 marked an inline comment as done.Aug 10 2016, 7:24 AM
alexfh added inline comments.Aug 10 2016, 10:33 AM
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
83 ↗(On Diff #67519)

No danger of dangling references here, since StringMap copies keys. See StringMap comments and StringMapEntry::Create implementation.

So there's no need to store another copy of this data.

Kirill, please revert the latest change in this constructor.

omtcyf0 updated this revision to Diff 67551.Aug 10 2016, 10:52 AM
omtcyf0 edited edge metadata.
omtcyf0 marked an inline comment as not done.

Revert changes.

Revert changes.

Thanks! If Aaron has no objections, good to go.

Revert changes.

Thanks! If Aaron has no objections, good to go.

Great! Thank you!

This revision was automatically updated to reflect the committed changes.