[cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy

[David Blaikie via cfe-dev]

I'm a bit confused by this whole discussion.

clang-format is neither mandated (by documentation) nor enforced (by any
infrastructure/automation) for use in the LLVM project that I know of.

It's convenient, and in review people may reasonably ask authors to
consider running it, etc - but we have no system that requires or checks
for that. Might be nice, might not be.

It sounds like even if clang-format were mandated - we mostly accept that
such a mandate is forward-looking, and that we don't want/intend to
reformat the entire existing codebase. So I imagine the same would apply to
clang-tidy (we wouldn't expect to tidy the entire codebase - we'd just use
clang-tidy as advisory for any new changes as we do with clang-format) - so
I don't think it'd run up against Danny's protest about choosing a
philosophy for wide scale changes, because there would be no wide scale
changes.

All that said, I think like clang-format we should probably have a
project-wide config that specifies which checks we care about - but the
real impediment to adoption of those checks is a streamlined process for
running them on a patch. clang-format has nice integration with my editor
(vim) and source control system (git-clang-format). I have yet to discover
an equally convenient place to put clang-tidy in my workflow. Without that
I doubt it'll see very much adoption within the LLVM community.

- Dave

On Thu, Dec 29, 2016 at 4:24 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> Hi everyone,
> I would like to start a discussion about enforcing use of clang-tidy (or
> better clang-tidy-diff) on patches before sending it to review.
> I like how clang-format simplified sending patches and reviews, e.g. "Use
> clang-format" instead of giving long list of lines that should be formatted
> by reviewer.
> I believe that clang-tidy can be also be very helpful here.
> Note that by enforcing I mean the same way as we enforce clang-format - It
> is not about "every changed line need to be formatted by clang-format"
> because thee might be some special formatting that looks better
> formatted by hand, it is about saving time for 99.9% of the lines. The
> same applies to clang-tidy, where there might be some bugs or false
> positives, but I would like to save my time as reviewer to not mention
> things like "use auto", "use range loop", "pass by value" over and over.
>
> But before enforcing clang-tidy, we have to get to consensus on adding new
> rules to coding style.
> I will list clang-tidy checks that I think should be used in LLVM. To make
> it short I will not justify why it is better to use this style because the
> documentation of checks is doing good job there.
>
> List of checks that I would like to add to .clang-tidy config
> modernize-*
>
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-redundant-void-arg.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-auto-ptr.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-shrink-to-fit.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html
>
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html
> http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html
>
>
> I skipped some checks because they work only in C++14.
> Coding style right now only mention using auto.
>
> http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> Loops are mentioned here:
>
> http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
> - but thi probably should be changed to "use for range loop if possible.
> Try to not evaluate end() multiple times if for range loop is not possible
> to use"
>
>
> I don't know which of the listed changes sounds controversial to you, so
> if you would like to start discussion about one of them, then please also
> mention that you agree/disagree with the others.
> One of the things that I would like to propose, is using push_back
> whenever the inserted type is the same or it implicitly casts itself.
>
> std::vector<Value*> v;
> Instr *I;
> Value *V;
> IntrinsicInstr *II;
> v.push_back(I);
> v.push_back(V);
> v.push_back(II);
>
> Use emplace_back only if we insert temporary object like:
>
> std::vector<SomeType> v2;
> v2.emplace_back(a, b, c); // instead of v.push_back(SomeType(a, b, c));
>
> The first reason is make code simple and more readable. I belive that code
> 'v.push_back(I)' is more readable than 'v.emplace_back(I)' because I don't
> have to think about if the element type has special constructor taking
> Instr, or it is just vector of Instr.
> From yesterday discussion, Daniel Berlin proposed using emplace_back
> everywhere to make code easier to maintain. I think it is valid argument,
> but I believe it would reduce readability.
> There is also other reason - emplace_back can't take arguments of some
> types like bitfields, NULL, static member. Using emplace can also lead to
> memory leak in case of smart ptrs
> "
>
> auto *ptr = new int(1);v.push_back(std::unique_ptr<int>(ptr));
>
> This is because replacing it with emplace_back could cause a leak of this
> pointer if emplace_back would throw exception before emplacement (e.g.
> not enough memory to add new element).".
>
> In the case of push_back/emplace_back for the same type, there should be
> no performance changes, however emplace_back might generate more template
> instantiations slowing down compilation.
>
> There is also topic of using insert/emplace for maps showing that
> map.emplace can be slower than map.insert. I would not want to distinguish
> between emplace for maps and emplace for vectors,
>
> so I believe using emplace for constructed temporaries, even if there
> would be some slightly performance regression, looks simpler and more
> readable.
>
>
>
> I am happy to change to write changes to LLVM coding standards document,
> but I firstly would like to see what community thinks about it.
>
> NOTE that I don't propose running clang-tidy and modifying whole codebase.
> It might happen some day, but we firstly need to agree that these changes
> are better.
>
> There are also other checks from performance and misc that I would like to
> enable. Some of them are already enabled (all misc), but because they are
> more about finding bugs, I would like to narrow discussion only to
> modernize.
>
> Piotr
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161229/16f1b20c/attachment.html>