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

[Sean Silva via cfe-dev]

On Thu, Dec 29, 2016 at 9:34 AM, David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> 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.
>

+1

I don't remember there being a discussion analogous to the "enforcement"
aspect of thread for clang-format. The uptake of clang-format was organic.

Piotr, what is different about the situation with clang-tidy (vs
clang-format) that we need to have any discussion of "enforcement" in this
thread? I think it's totally reasonable to have a discussion of which
checks we want to run by default for the "LLVM style" (to use
clang-format's terminology), and this thread should probably focus on that.

wrt adding rules to the coding standards, I don't think we want to clutter
the coding standards document with a specific rule for every clang-tidy
check we want to have on by default for the "LLVM style", the same way that
we don't have a rule in the coding standards document for every
clang-format config value which happens to be set in the "LLVM style" (i.e.
all the stuff you see when you do -dump-config). Just the main ones is fine.

-- Sean Silva


>
> - 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
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20161229/70b9063d/attachment.html>