This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Apply clang-format to GenericTaintChecker.cpp
ClosedPublic

Authored by boga95 on Nov 26 2018, 2:30 PM.

Diff Detail

Repository
rC Clang

Event Timeline

boga95 created this revision.Nov 26 2018, 2:30 PM
gerazo accepted this revision.Nov 27 2018, 3:10 AM
gerazo added a subscriber: xazax.hun.

Looks correct clang-format to me.

This revision is now accepted and ready to land.Nov 27 2018, 3:10 AM

If you find it ok, can you please commit it? @xazax.hun

Szelethus retitled this revision from Apply clang-format to GenericTaintChecker.cpp to [analyzer] Apply clang-format to GenericTaintChecker.cpp.Dec 15 2018, 2:26 AM
Szelethus added reviewers: Szelethus, NoQ, xazax.hun.

Please don't mind me intruding.
If you create a patch for the Clang Static Analyzer, please add "[analyzer]" in the revision title, because many of us are automatically subscribed to those patches. Also, upload patches with full context.
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

I see the point here, and I think it'd be worth to just clang-format most of the files we work with, as the benefits would outweigh the drawbacks (at least eventually...), but I vaguely remember @george.karpenkov having many great points against it -- please don't commit until he can take a look :)

Szelethus accepted this revision.Dec 15 2018, 3:19 AM

Hmm, actually, if you're doing changes all over the file in the followup patches, it shouldn't matter much. Let's just wait a couple days for ppl to object, now that they are subscribed.

NoQ accepted this revision.Dec 16 2018, 4:03 PM

I appreciate the cleanup in general and any work on taint analysis in particular, thanks!

I vaguely remember @george.karpenkov having many great points against it -- please don't commit until he can take a look :)

Essentially, it messes with git blame, and also causes a lot of merge conflicts for downstream developers (we have a few of those). But if you plan to work on this checker actively anyway, please feel free to start with formatting.

Here's the relevant quote from the guidelines:

Our long term goal is for the entire codebase to follow the convention, but we explicitly *do not* want patches that do large-scale reformatting of existing code. On the other hand, it is reasonable to rename the methods of a class if you’re about to change it in some other way. Just do the reformatting as a separate commit from the functionality change.

@boga95 Do you have commit access, or need this to be commited on your behalf?

I don't have commit access. Can you please do it on my behalf?

This revision was automatically updated to reflect the committed changes.