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

[David Blaikie via llvm-commits]

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.

- Dave



>
> -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/20151017/1678bc25/attachment.html>