[cfe-dev] Cleanup on aisle DebugInfo enums

[Eric Christopher via cfe-dev]

On Fri, Feb 5, 2016 at 2:11 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Feb 5, 2016 at 11:30 AM, Robinson, Paul <
> Paul_Robinson at playstation.sony.com> wrote:
>
>> what's the motivation not to use the LLVM versions of these enums
>>
>>
>>
>> I originally implemented the Clang bits for debugger tuning using the
>> LLVM enum (r256063).  However this caused build failures in
>> clang-tools-extra (which I didn't have checked out locally) because having
>> CodeGenOptions.h pull in the extra LLVM header caused confusion in some
>> unqualified name references.
>>
>> Rather than introduce the extra qualifiers in clang-tools-extra (I
>> looked, it would have taken a bit to sort everything out) I undid the
>> dependency on the LLVM headers and just replicated the enum in Clang.  The
>> only place in Clang that still "really" needed the LLVM enum is
>> BackendUtil.cpp (natch) and it is careful not to assume the two enums are
>> defined the same way.
>>
>
> Still seems a bit subtle - are there reasons we should avoid using the
> LLVM enum apart from the unknown issue with clang-tools-extra? (if we
> generally keep these types/enums separate, that's OK - but don't want to
> propagate a workaround if we don't fully understand what we're working
> around)
>
>

We've kept a bunch of the target stuff separate, but I don't see any reason
to not use the general dwarf enums. I think we should just fix the
clang-tools-extra stuff.

-eric


> This got committed in r256078.
>>
>> However, I wasn't thorough enough about digging out the LLVM
>> dependencies, and Driver is still using the LLVM enum.
>>
>> If I'd gotten the Driver part right the first time around, Benny would've
>> had to move both enums in his patch.
>>
>>
>>
>> This Clang-versus-LLVM consideration applies only to the tuning enum,
>> btw; LLVM doesn't have an analog of DebugInfoKind.
>>
>> --paulr
>>
>>
>>
>> *From:* David Blaikie [mailto:dblaikie at gmail.com]
>> *Sent:* Friday, February 05, 2016 11:06 AM
>> *To:* Robinson, Paul; Adrian Prantl; Eric Christopher; Benjamin Kramer
>> *Cc:* cfe-dev at lists.llvm.org
>> *Subject:* Re: [cfe-dev] Cleanup on aisle DebugInfo enums
>>
>>
>>
>> Sounds plausible.
>>
>> (just to check: what's the motivation not to use the LLVM versions of
>> these enums if they match the desired semantics? (I imagine there are many
>> good reasons not to use them, just figure it's worth writing down in this
>> thread so it's clear for historic purposes))
>>
>>
>>
>> On Fri, Feb 5, 2016 at 10:57 AM, Robinson, Paul via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>> In r259489, Benny Kramer moved the DebugInfoKind enum into its own
>> header, include/clang/Driver/DebugInfoKind.h. This was to prevent
>> Driver from pulling in headers from Frontend.  However, this leaves
>> the enum in kind of a weird place, I think Basic would be better (and
>> not re-introduce the problem Benny was solving).
>>
>> Wondering why the tuning enum didn't cause the same problem, I found
>> that when I created the Clang enum, I didn't make Driver use it; it's
>> still using the LLVM version of the enum.  While this is not
>> technically wrong (you could rearrange the values and it would do no
>> harm) it's an inconsistency within Clang.
>>
>> But, if I make Driver use the tuning enum from Clang's CodeGenOptions,
>> then we'll have the same problem that prompted r259489 in the first
>> place--Driver depending on Frontend. Rather than create Yet Another
>> Header, it would be convenient to put the tuning enum in the same
>> place as DebugInfoKind, as they are topically related and commonly
>> used in the same places.
>>
>> I propose to do the following:
>>
>> Patch #1 (totally mechanical)
>> - move DebugInfoKind.h from Driver to Basic
>> - rename it to DebugInfoOptions.h
>> This will take care of all the #include renaming in one go.
>>
>> Patch #2 (basically mechanical)
>> - move the DebuggerKind enum from CodeGenOptions.h to DebugInfoOptions.h
>> - fix up Driver to use this enum instead of the LLVM equivalent.
>>
>> Or I can do it all at once, the combined patch would still be
>> reasonable I think.
>>
>> Sound okay?
>> --paulr
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160205/ceaa6426/attachment.html>