[PATCH] D23434: Don't allow llvm-include-order to intermingle includes from different files.

[Daniel Jasper via cfe-commits]

On Fri, Aug 12, 2016 at 8:22 PM, Zachary Turner <zturner at google.com> wrote:

> Sounds good.  Just to be clear, you plan to delete the code from
> clang-tidy, then take the code from clang-format and move it to clang-tidy,
> and have clang-format call clang-tidy (or otherwise share the code somehow
> so they both use the same implementation)?
>

No. clang-tidy just calls into clang-format (the library, not the binary)
and if clang-format would change the order of #includes, then clang-tidy
will display a warning. I already have implemented this. Will try to clean
it up and send out a patch.

I may still try to implement cross-block reordering in clang-tidy because
> it's the only way to do it in such a way that it just warns you but doesn't
> apply fixits, or applies them only if it doesn't break the compile, which
> is what I need at the moment.  But since this is just experimental anyway,
> it probably shouldn't matter much.  And if I end up getting something that
> works reasonably well, we can always move that code over to clang-format if
> we want to support cross-block reordering there.
>

It's highly unlikely that we'll be able to share code here. The way
#includes are detected and handled are very different between clang-format
and clang-tidy. If you want to try to implement this for clang-format, by
all means. If you are thinking of implementing this for clang-tidy, just
take what we have tried before. I can send you pointers to the code
(although it has never made it to upstream clang-tidy).

On Fri, Aug 12, 2016 at 11:17 AM Daniel Jasper <djasper at google.com> wrote:
>
>> I haven't read the patch, but if Alex is ok, so am I.. just wanted to
>> make sure that we don't spend much more time on this, as we are likely
>> going to remove most of the code..
>>
>> On Aug 12, 2016 6:42 PM, "Zachary Turner" <zturner at google.com> wrote:
>>
>>> Ahh, I see.  Just to be clear, is there an LGTM to get this path in?  I
>>> know alexfh@ lgtm'ed it, want to make sure you're ok with this too.
>>>
>>> On Fri, Aug 12, 2016 at 9:40 AM Daniel Jasper <djasper at google.com>
>>> wrote:
>>>
>>>> The check's implementation will be replaced by a simple call to clang
>>>> tidy. It will remain a check in clang tidy to continue to cater to both use
>>>> cases.
>>>>
>>>> On Aug 12, 2016 6:19 PM, "Zachary Turner" <zturner at google.com> wrote:
>>>>
>>>>> That's actually the reason I think it makes more sense in clang tidy.
>>>>> It can be a configuration option, off by default, and since there is more
>>>>> control over whether to apply fixits, and it doesn't apply fixits by
>>>>> default, it would be easier to iterate on the experimental nature of it
>>>>> without messing up code
>>>>>
>>>>>
>>>>> On Fri, Aug 12, 2016 at 9:14 AM Alexander Kornienko <alexfh at google.com>
>>>>> wrote:
>>>>>
>>>>>> alexfh added a comment.
>>>>>>
>>>>>> In https://reviews.llvm.org/D23434#513839, @djasper wrote:
>>>>>>
>>>>>> > I think we got confused. We once had tried to write an experimental
>>>>>> separate check to comply with Google's style guide. If you want to fiddle
>>>>>> around with that, contact me, I can send you pointers. But as I mentioned
>>>>>> we moved away from that. And I think it makes more sense to re-create the
>>>>>> sort-across-blocks functionality in clang-format and not in clang-tidy.
>>>>>>
>>>>>>
>>>>>> Yep, we definitely got confused. That experimental check actually
>>>>>> implemented cross-block sorting, but this caused a bunch of issues. Which
>>>>>> makes me think that proper implementation of cross-block include sorting is
>>>>>> challenging be it in clang-format or clang-tidy. Clang-format probably
>>>>>> makes it even more complex, since a higher safety of transformations is
>>>>>> expected from it.
>>>>>>
>>>>>>
>>>>>> https://reviews.llvm.org/D23434
>>>>>>
>>>>>>
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160813/c0a3269f/attachment-0001.html>