Page MenuHomePhabricator

[LibTooling][Clang-cast] A Clang LibTool to convert C-style casts to C++ style casts and more.
Needs ReviewPublic

Authored by oneraynyday on Oct 19 2020, 10:54 PM.

Details

Summary

Clang-cast is a clang tool that visits the clang AST to emit diagnostics about C style casts.
It classifies a C style cast as one that includes any combination of const/reinterpret/static casts,
one that can't be converted to a C++ cast, or a no-op cast. The tool can...

  • Either warn or error on particular types of C-style casts listed above.
  • Fix particular types of C-style casts listed above (in-place or in new files)
    • Can emit code that's -pedantic compliant (for e.x. VLA's are used in casting)
  • Give a summary of statistics of C style casts in the translation unit.
  • Warn on subtleties such as member-pointer casts of different classes and more.

The tool will stop at system headers and will not visit any headers if --no-includes is set.

I also apologize for the long PR. I was developing it in my own repository here to dogfood the tool with a small set of users. Here are the most important files:

  • Cast.h contains a class responsible for capturing a CStyleCastExpr node and deciding what C++ casts belong to it.
  • CastOptions.h contains a set of enums and options that the users interact with in the CLI.
  • ClangCast.cpp is the driver, defining the AST action.
  • Matcher.h is the match callback, responsible for most of the diagnostics and handling of user input.
NOTE: The tool differs a bit compared to clang-tidy in terms of FixIt reports. Clang-tidy currently can only diagnose up to warnings, otherwise the FixItRewriter will issue a note saying "FIX-IT detected an error it cannot fix" (in the code it's note_fixit_unfixed_error). This tool aims to be used in the CI pipeline to return a non-zero error code if --err-<type> is passed in as a flag. We also interchange the DiagnosticConsumer so that we can print FixIt's for all recommendations but have the flexibility to edit a few with the --fix-<type> flags passed in. In clang-tidy, this is usually done by having a logical branch and not printing the FixIt's. I personally think the FixIt's are visually helpful and want to keep them there even when the changes are not applied. However, since most of these functions are modularized, I can create an equivalent addition to clang-tidy with its own diagnostics format, and perhaps a subset of the features to keep it less complicated compared to typical clang-tidy subtools (for example, only warnings, no summary, etc)

Diff Detail

Unit TestsFailed

TimeTest
50 mswindows > Extra Tools Unit Tests.clang-cast/_/ClangCastTests_exe::ClangCXXCastTest.TestCStyleCastTypes
Note: Google Test filter = ClangCXXCastTest.TestCStyleCastTypes [==========] Running 1 test from 1 test case.

Event Timeline

oneraynyday created this revision.Oct 19 2020, 10:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2020, 10:54 PM
oneraynyday requested review of this revision.Oct 19 2020, 10:54 PM
oneraynyday edited the summary of this revision. (Show Details)

Updated description

oneraynyday edited the summary of this revision. (Show Details)Oct 19 2020, 11:22 PM
oneraynyday added a project: Restricted Project.

Don't use C++17 features as clang currently uses C++14.

Reminder to upload patches with diff to master, not last patch/commit/upload.

Wouldn't it be better for this to be a clang-tidy check, as opposed to a standalone tool?

Hi there,

Thank you for taking a look! Sorry about the mess with diff's - this is my first time working with Arcanist. I thought arc diff --update <ID> in Arcanist meant a push to the branch. Moving forward, should I use arc diff to add extra changes onto the code review?

Regarding clang-tidy, I listed some of the reasons why I currently have it formatted as a standalone tool in the NOTES section above.

Reverting previous update

  • Don't use C++17 features
oneraynyday edited the summary of this revision. (Show Details)Oct 20 2020, 12:54 AM

Hi @oneraynyday ! This looks very interesting - thanks for uploading! I've only quickly skimmed through. Two high level points:

  • Have you considered sending an RFC to cfe-dev regarding this tool? I think that it would be a great way of attracting peoples attention. More importantly, should we add another tool to clang-tools-extra?
  • Tthis is a rather large patch - could you split it into separate chunks? Otherwise it's quite tricky to review.

Thanks,
-Andrzej

  • Have you considered sending an RFC to cfe-dev regarding this tool? I think that it would be a great way of attracting peoples attention. More importantly, should we add another tool to clang-tools-extra?

Hi @awarzynski, thank you so much for taking a look! I was considering sending an RFC but haven't gotten around to it yet. Funny enough, how I started the development on this tool was because of this page: https://clang.llvm.org/docs/ClangTools.html which encourages newcomers to clang to create new tools. Specifically this line:

C++ cast conversion tool. Will convert C-style casts ((type) value) to appropriate C++ cast (static_cast, const_cast or reinterpret_cast).

I did exactly this project(was much bigger of an undertaking than I had first imagined) and was hoping to contribute back, assuming that everyone would be fairly receptive to having an extra tool that helps the community :) In retrospect, I do realize that this could be a feature in clang-tidy and I noted in the NOTES section that I can contribute a subset of the functionalities into clang-tidy as well.

  • [This] is a rather large patch - could you split it into separate chunks? Otherwise it's quite tricky to review.

Of course, I'd be glad to make the review experience better for you. How do you want me to split it into chunks? I had the following in mind:

  • First patch: CStyleCastOperator class in Cast.h (with all the unit tests)
  • Second patch: Creating the minimal matcher, consumer, action and necessary CLI options
  • Third patch: Adding in the matcher implementation which emits diagnostics.

Let me know what you think.

  • Address clang-tidy
  • Address using C++17 left-fold expression in C++14
  • Address minor style issues
  • It appears symlinks are directly copied over, here are the actual file contents
  • Address clang-tidy warnings
  • Duplicated C-style cast removed (why is this failing in windows?)
  • Addressed clang-tidy
  • Windows tests doesn't classify the test case as a dependent cast
  • Wrong test name
  • Retain (void) casts
  • Fix bug where we didn't realize ImplicitCastExpr is doing all of the cast in the CStyleCastExpr in clang
  • Typedef wrapping whenever possible