[llvm] r250459 - Replace a forward declaration with an #include.

[Eric Christopher via llvm-commits]

On Sat, Oct 17, 2015 at 1:26 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Oct 16, 2015 at 4:51 PM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>>
>>
>> On Fri, Oct 16, 2015 at 3:52 PM David Blaikie <dblaikie at gmail.com> wrote:
>>
>>> On Fri, Oct 16, 2015 at 10:19 AM, Eric Christopher via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Oct 16, 2015 at 10:18 AM Eric Christopher <echristo at gmail.com>
>>>> wrote:
>>>>
>>>>> On Fri, Oct 16, 2015 at 10:14 AM Adrian Prantl <aprantl at apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Oct 16, 2015, at 9:56 AM, Eric Christopher <echristo at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>> Eh? Why?
>>>>>>
>>>>>>
>>>>>> The file forward-declares DebugLocStream and then uses the inner type
>>>>>> (DebugLocStream::ListBuilder *) in an argument list. As noted in the commit
>>>>>> message, clang complains about this when building with modules.
>>>>>>
>>>>>>
>>>>> This seems odd.
>>>>>
>>>>>
>>>>>> I’m unsure whether this is supposed to work or if I was just working
>>>>>> around a bug in clang. If this is indeed a clang bug, we should fix it
>>>>>> instead.
>>>>>>
>>>>>
>>>>> Right. This is what I'm asking.
>>>>>
>>>>
>>>> And if it isn't going to work in modules it should probably warn/error
>>>> normally as well.
>>>>
>>>
>>> Modules allows us to find errors we can't diagnose without them.
>>>
>>> In this case, chances are this header is included in a context that
>>> already included DebugLocStream.h before it - the header wasn't standalone.
>>> In a non-modular build we don't diagnose non-standalone headers.
>>>
>>>
>> *nod* That makes perfect sense, just wasn't clear from the commit message
>> and no one has tracked down if that is the case or not :)
>>
>
> Yeah, commit message could've been clearer.
>
> If you revert the patch then move DebugLocEntry.h's inclusion in
> DwarfDebug.cpp up to the top of the inclusions, you get this error:
>
> In file included from /usr/local/google/home/blaikie/dev/llvm/src/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:14:
> /usr/local/google/home/blaikie/dev/llvm/src/lib/CodeGen/AsmPrinter/DebugLocEntry.h:146:39: error: incomplete type 'llvm::DebugLocStream' named in nested name specifier
>   void finalize(const AsmPrinter &AP, DebugLocStream::ListBuilder &List,
>                                       ^~~~~~~~~~~~~~~~
> /usr/local/google/home/blaikie/dev/llvm/src/lib/CodeGen/AsmPrinter/DebugLocEntry.h:21:7: note: forward declaration of 'llvm::DebugLocStream'
> class DebugLocStream;
>       ^
>
> That error is not present if you keep this patch in and apply the same include order change.
>
> These errors are usually caught by a non-modular build for any .h file that has an associated .cpp (because the style rule is to include the corresponding .h first for precisely this reason). DebugLocEntry.h doesn't have a corresponding .cpp, it's only included in DwarfDebug.cpp (and it does have one out-of-line member function defined there) which has DwarfDebug.h first.
>
> Yep. Makes sense. Thanks for looking.

-eric


>
>> -eric
>>
>>
>>> Only once you go to build the header as a separate module (or submodule)
>>> do you see this.
>>>
>>> So I /think/ this expected behavior & the sort of thing we expect the
>>> modules buildbot to find & cause us to fix.
>>>
>>> (cc'd Richard in case I've missed something here)
>>>
>>> - Dave
>>>
>>>
>>>>
>>>> -eric
>>>>
>>>>
>>>>>
>>>>> -eric
>>>>>
>>>>>
>>>>>>
>>>>>> -- adrian
>>>>>>
>>>>>>
>>>>>> -eric
>>>>>>
>>>>>> On Thu, Oct 15, 2015 at 2:00 PM Adrian Prantl via llvm-commits <
>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Author: adrian
>>>>>>> Date: Thu Oct 15 15:58:55 2015
>>>>>>> New Revision: 250459
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=250459&view=rev
>>>>>>> Log:
>>>>>>> Replace a forward declaration with an #include.
>>>>>>> When building with modules the forward-declared inner class
>>>>>>> DebugLocStream::ListBuilder causes clang to fall over.
>>>>>>>
>>>>>>> Modified:
>>>>>>>     llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h
>>>>>>>
>>>>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h?rev=250459&r1=250458&r2=250459&view=diff
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h (original)
>>>>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DebugLocEntry.h Thu Oct 15
>>>>>>> 15:58:55 2015
>>>>>>> @@ -9,6 +9,8 @@
>>>>>>>
>>>>>>>  #ifndef LLVM_LIB_CODEGEN_ASMPRINTER_DEBUGLOCENTRY_H
>>>>>>>  #define LLVM_LIB_CODEGEN_ASMPRINTER_DEBUGLOCENTRY_H
>>>>>>> +
>>>>>>> +#include "DebugLocStream.h"
>>>>>>>  #include "llvm/ADT/SmallString.h"
>>>>>>>  #include "llvm/IR/Constants.h"
>>>>>>>  #include "llvm/IR/DebugInfo.h"
>>>>>>> @@ -17,7 +19,6 @@
>>>>>>>
>>>>>>>  namespace llvm {
>>>>>>>  class AsmPrinter;
>>>>>>> -class DebugLocStream;
>>>>>>>
>>>>>>>  /// \brief This struct describes location entries emitted in the
>>>>>>> .debug_loc
>>>>>>>  /// section.
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151021/a9da03ff/attachment-0001.html>