[cfe-dev] [PATCH] Fixit for incorrect includes

[Aaron Ballman]

On Tue, Jul 17, 2012 at 12:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Tue, Jul 17, 2012 at 10:48 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Tue, Jul 17, 2012 at 12:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Tue, Jul 17, 2012 at 9:14 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>> This patch creates a fixit for include directives where the file could
>>>> not be found when using angle brackets, but can be found when using
>>>> quotes.  The converse is not needed since quoted includes will search
>>>> angle bracket locations by default.
>>>>
>>>> Eg)
>>>> #include <header.h>  // can be found via #include "header.h" instead
>>>>
>>>> Thoughts?
>>>
>>> Seems like a neat idea to me - but I'm not an authoritative sign-off.
>>>
>>> You used NULL as null constants for 2 of the conditional operators,
>>> then 0 for the third - that seems inconsistent. You might want to
>>> check what the prevailing style is in this file & stick to that
>>> (generally in LLVM, '0' seems to be winning as the authoritative null
>>> pointer constant, I believe).
>>
>> Easy enough to rectify (I'll standardize on 0) -- this was a copy from
>> above, so I'll fix up there as well.
>>
>>> Did you consider adding this case to the existing fixit testing files?
>>> They're already a grab-bag of things that can be fixed (this helps
>>> keep the test suite fast by not adding more separate test file
>>> executions) & this seems like it'd be at home there.
>>
>> I thought about it, but it strikes me as different enough to warrant
>> its own test case.  Specifically, the fixit behavior is kind of
>> tricky.  Because the fixit writes out a separate file, but in a
>> different directory, I have to be a bit sneaky otherwise the header
>> file isn't available.  This is because the temp file goes to the
>> Output directory, but the header file remains in the FixIt directory.
>> To work around this, I copy the testcase to a temp file, modify the
>> testcase file directly with the fixit,
>
> Why can't you modify the temp file directly instead? (& you'd have to
> copy the header over too, I suppose - you can identify the temp
> directory with %T which means you can copy to fixed name files if
> that's important (as it would be for the header name): %T/foo.cpp or
> whatever)

You learn something new every day!  I didn't realize you could use %T
for the temp directory.  Now the testcase looks like:

// RUN: %clang_cc1 -fsyntax-only -verify %s
// RUN: cp %s %t
// RUN: cp %S/fixit-include.h %T
// RUN: not %clang_cc1 -fsyntax-only -fixit %t
// RUN: %clang_cc1 -Wall -pedantic %t

Which is cleaner (to me).  Thanks!

> & yeah, arguable whether the existing fixit cases should be burdened
> with this mucking about - I don't mind too strongly either way.

Since include errors are fatal, I figured this was a clean separation.
 I'm not strongly tied to it though.

New patch attached.

~Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_fixit.patch
Type: application/octet-stream
Size: 2061 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120717/0b5cac2f/attachment.obj>