Page MenuHomePhabricator

[clang-tidy] introduce modernize-deprecated-headers check
ClosedPublic

Authored by omtcyf0 on Feb 20 2016, 11:14 AM.

Details

Summary

This patch introduces the modernize-deprecated-headers check, which is supposed to replace deprecated C library headers with the C++ STL-ones.

For information see documentation; for exmaples see the test cases.

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyf0 updated this revision to Diff 48597.Feb 20 2016, 11:14 AM
omtcyf0 retitled this revision from to [clang-tidy] introduce modernize-deprecated-headers check.
omtcyf0 updated this object.
omtcyf0 added a subscriber: cfe-commits.
omtcyf0 updated this revision to Diff 48598.Feb 20 2016, 11:17 AM

fixed clang-tidy/modernize/DeprecatedHeaders.h head typo caused by the check rename

Thank you for implementing this check!

However, I think will be good idea to always wrap header name in <> for warning and FixIt.

omtcyf0 updated this revision to Diff 48600.Feb 20 2016, 2:36 PM
omtcyf0 edited edge metadata.

Thanks for a review, Eugene!

The FixItHint and warning now comes with the "angled" version of header wrapping.

Other than a few nits, LGTM.

clang-tidy/modernize/DeprecatedHeadersCheck.cpp
54 ↗(On Diff #48600)

Can't we use C++11 brace initialization here instead of repeated assignments?

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

Can we sort these lists of includes? Otherwise it's hard to find a specific include in the list.

test/clang-tidy/modernize-deprecated-headers-cxx11.cpp
3 ↗(On Diff #48600)

Can we sort the includes in the test files too please?

Another idea: to replace limits.h with limits and also replace its defines with their C++ counterparts. For example, INT_MIN with numeric_limits<int>::min().

Will be definitely useful for LLDB code modernization.

omtcyf0 updated this revision to Diff 48613.Feb 21 2016, 3:01 AM
omtcyf0 edited edge metadata.

Thanks for a review, Richard!
Fixed all the issues you pointed to!

Thanks for the hint, Eugene!
I'll try to add this functionality to this check later on.

omtcyf0 marked 3 inline comments as done.Feb 21 2016, 3:02 AM
alexfh edited edge metadata.Feb 22 2016, 7:59 AM

Thank you for this check! Mostly looks good, but there are a number of style nits.

The most important question to this check is that the standard doesn't guarantee that the C++ headers declare all the same functions in the global namespace (at least, this is how I understand the section of the standard you quoted). This means that the check in its current form can break the code that uses library symbols from the global namespace. The check could add std:: wherever it's needed (though it may be not completely trivial) to mitigate the risk. It's not what needs to be addressed in the first iteration, but it's definitely worth a look at in order to make the check actually useful. It also could be a separate check or a separate mode (configurable via parameters) of this check, while someone could have reasons to do the migration in two stages (add std:: qualifiers and then switch to new headers).

clang-tidy/modernize/DeprecatedHeadersCheck.cpp
15 ↗(On Diff #48613)

Is this header used?

36 ↗(On Diff #48613)

There's a more effective container in LLVM: llvm::StringMap<std::string>. If your code guarantees that only string literals be used to initialize this map, this can be a llvm::StringMap<StringRef>.

54 ↗(On Diff #48613)

It should be possible to initialize this in the constructor initializer list.

77 ↗(On Diff #48613)

nit: Please add a trailing period.

94 ↗(On Diff #48613)
  1. s/std::string(FileName)/FileName.str()/
  2. no need for this, if llvm::StringMap is used
96 ↗(On Diff #48613)

FileName.str()

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

This quote from the standard is too long for my taste. A simple reference to the relevant section should be enough ([depr.c.headers]).

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

You seem to have forgotten to add these headers to Inputs/Headers. Or doesn't the check care about them actually being present?

25 ↗(On Diff #48613)

Trailing period.

32 ↗(On Diff #48613)

Please also check diagnostic messages (// CHECK-MESSAGES: ...).

... the check in its current form can break the code that uses library symbols from the global namespace. ...

An action item for this is: document this possible issue and add a FIXME somewhere in the code to add std:: qualifiers.

Thank you for this check! Mostly looks good, but there are a number of style nits.

The most important question to this check is that the standard doesn't guarantee that the C++ headers declare all the same functions in the global namespace (at least, this is how I understand the section of the standard you quoted).

Oh crap, I totally forgot about this. Yes, I believe the weasel wording in the standard is that an implementation may put the symbols in the global namespace as well as inside std when the C++ header is used, but is not required to put them in the global namespace. They are required to put them in namespace std when the C++ header form is used.

So if you switch to C++ headers, you code may still compile, but it is implementation dependent.

If you use the C headers, you can't prefix symbols with std because they won't be there.

In other discussions of this topic, it was considered best to make both changes at the same time: switch to the C++ header and decorate the symbols with std prefix.

It would be reasonable for this check to insert a using namespace std; at the top of the translation unit after all the #include lines as a means of preserving meaning without trying to identify every symbol that needs std on it.

Thank you for a review, Alex!

I'll clean up the parts you mentioned soon!

The most important question to this check is that the standard doesn't guarantee that the C++ headers declare all the same functions in the global namespace (at least, this is how I understand the section of the standard you quoted). This means that the check in its current form can break the code that uses library symbols from the global namespace. The check could add std:: wherever it's needed (though it may be not completely trivial) to mitigate the risk. It's not what needs to be addressed in the first iteration, but it's definitely worth a look at in order to make the check actually useful. It also could be a separate check or a separate mode (configurable via parameters) of this check, while someone could have reasons to do the migration in two stages (add std:: qualifiers and then switch to new headers).

Yes, you are correct: the check might cause some troubles in codebases, because it is not guaranteed that the functions from the C library headers will occur in the global namespace.

I was initially aware of that, but I thought it'd be better to add the initial check first and enhance it later. Appropriate FIXME seems great.

It would be reasonable for this check to insert a using namespace std; at the top of the translation unit after all the #include lines as a means of preserving meaning without trying to identify every symbol that needs std on it.

I'm not sure that's a good option. I would certainly argue about putting using namespaces like std anywhere during the vanilla check run. Simply because of the collisions caused by some libraries with the functions having analogies in STL.

However, I can see it as an option for the further variations of this check.

Say, let it have three options:

  • Don't bother about namespaces: in case user already has using namespace std or doesn't care about implementation-dependend things. Seems like a reasonable scenario to me.
  • Add std:: prefix to all functions from these headers in the process of migration: generally it seems a bit better to me.
  • Put using namespace std; somewhere (maybe even specify where: i.e. if user wants to put it right to the top of TU or in each source file where a C library is used. That's considerably good, too.

just note for the future
I also think there might be a good place in clang-tidy for an independent check responsible for namespace migrations. Say, user has been using only one library and now when he wants to add another one it would be good to put the appropriate namespace prefixes where appropriate.

Seems quite sophisticated ATM though.

omtcyf0 updated this revision to Diff 48711.Feb 22 2016, 11:10 AM
omtcyf0 edited edge metadata.
omtcyf0 marked 9 inline comments as done.

Fixed all the issues @alexfh pointed to.

Marked the comments as *done*.

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

Correct. It doesn't care about them being present.

alexfh added inline comments.Feb 22 2016, 3:23 PM
clang-tidy/modernize/DeprecatedHeadersCheck.cpp
78 ↗(On Diff #48711)

This can be done without STL algorithms and lambdas:

for (const auto &Pair : {{"fenv.h", "cfenv"}, ...})
  CStyledHeaderToCxx.insert(Pair);

(didn't actually try this, so it might need some massaging, e.g. std::pair<....> instead of auto).

104 ↗(On Diff #48711)

A usual way to concatenate strings in LLVM is to use llvm::Twine:

std::string Replacement = (llvm::Twine("<") + ... + ...).str();
105 ↗(On Diff #48711)

The message should name the header it complains about and suggest the replacement (in words, not only by attaching a fix. Something along the lines of:

"inclusion of  a deprecated C++ header '%0'; consider including '%1' instead"
test/clang-tidy/modernize-deprecated-headers-cxx03.cpp
32 ↗(On Diff #48711)

We usually specify each unique diagnostic message completely in one CHECK line and truncate repeated parts (e.g. the check name in brackets) in all other CHECK lines to make tests easier to read.

omtcyf0 updated this revision to Diff 48797.Feb 23 2016, 2:10 AM
omtcyf0 marked 4 inline comments as done.

Resolved issues @alexfh pointed to.

omtcyf0 updated this revision to Diff 48799.Feb 23 2016, 2:18 AM

changed the warning comment to be more precise; fixed line with > 80 symbols

alexfh accepted this revision.Feb 23 2016, 5:47 AM
alexfh edited edge metadata.

Looks good with one nit.

Let me know if you need me to commit the patch for you.

clang-tidy/modernize/DeprecatedHeadersCheck.cpp
99 ↗(On Diff #48799)

No need to convert all of them to llvm::Twine. The first one is enough to make the compiler choose the right overloaded operators:

(llvm::Twine("<") + CStyledHeaderToCxx[FileName] + ">").str();
This revision is now accepted and ready to land.Feb 23 2016, 5:47 AM
omtcyf0 updated this revision to Diff 48905.Feb 24 2016, 4:12 AM
omtcyf0 edited edge metadata.

replaced few explicit casts to llvm::Twine with implicit ones

Looks good with one nit.

Let me know if you need me to commit the patch for you.

Yes, commit please!

This revision was automatically updated to reflect the committed changes.