[lldb] Add include for SBLanguages in lldb-enumerations #111907
Conversation
|
@llvm/pr-subscribers-lldb Author: Chelsea Cassanova (chelcassanova) ChangesThis adds an include for SBLanguages.h in lldb-enumerations.h so that files that need this enum do not have to explicitly include SBLanguages. Full diff: https://github.com/llvm/llvm-project/pull/111907.diff 1 Files Affected:
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 938f6e3abe8f2a..df3aadcb0689c6 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -11,6 +11,7 @@
#include <cstdint>
#include <type_traits>
+#include <lldb/API/SBLanguages.h>
#ifndef SWIG
// Macro to enable bitmask operations on an enum. Without this, Enum | Enum
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
chelcassanova
force-pushed
the
add-sblanguages-include-to-enumerations
branch
from
133ace4 to
c179c08
Compare
This adds an include for SBLanguages.h in lldb-enumerations.h so that files that need this enum do not have to explicitly include SBLanguages.
chelcassanova
force-pushed
the
add-sblanguages-include-to-enumerations
branch
from
c179c08 to
e439392
Compare
|
@chelcassanova https://lab.llvm.org/staging/#/builders/195/builds/4266 i.e. when only 'lldb-server' target gets built ( |
)" Temporarily Revert until Chelsea can look at this. With a clean build, SBLanguages.h won't be generated in the build directory at the point when it is included by lldb-enumerations when compiling e.g. Broadcaster.cpp. On a clean build (no pre-existing build directory), the dependency ordering is not explicitly stated so the build will fail. An incremental build will succeed. This reverts commit b355426.
There was a problem hiding this comment.
This layering seems wrong to me. If you put an SB header in lldb-enumerations.h, then you are pretty much including it in all of lldb. But SB defines should really only be used in include/lldb/API and source/API.
Note, SBLanguages.h itself seems wrong to me. It is used to pass a language name enum to SBExpressionOptions::SetLanguage, which turns around and calls EvaluateExpressionOptions::SetLanguage, but in that latter API we call the parameter a uint16_t. Why didn't we make an lldb-languages.h and include that in some place that both the SB caller and the lldb API it wraps can share it on the up-and-up?
|
I was going to fix the dependency issue here to make sure that SBLanguages gets generated before trying to build any of these targets, but talking with @jimingham, this shouldn't have been included at all as |
|
My mental model is that everything in |
|
On Oct 10, 2024, at 4:14 PM, Alex Langford ***@***.***> wrote:
My mental model is that everything in lldb-enumerations.h should be public and all the private enumerations should go in lldb-private-enumerations.h. In practice I think there are tons of things in lldb-enumerations.h that should not be public at all, but I'm not sure which are safe to remove since they're all exposed through the Python bindings.
I agree with that, but the argument here was going the other way. lldb-enumerations.h are a base set of enumerations that anybody using LLDB, either lldb_private or the SB API's are free to use. But the SB API's are purely wrappers around lldb_private API's, and so should never be used in lldb_private. If you put an SB API header in lldb-enumerations.h, now most of lldb_private is going to see it.
Does that also fit wth your mental model?
Jim
… —
Reply to this email directly, view it on GitHub <#111907 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4MGWXIYXXUTNNN6NTZ24C3RAVCNFSM6AAAAABPXQYNLCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBWGE4TOMRWGU>.
You are receiving this because you were mentioned.
|
This patch moves `SBLanguages.h` out of the API tree. This file gets generated at build time using DWARF table-gen file and contains an enumeration of the DWARF supported source language names and there respective code/identifier. Since this is an enum, this shouldn't be part of the API tree but rather it should be included in the `lldb-enumeration` header. Also, that enum was used in internal methods in `lldb_private` by down-casting the enum value type to an `uint16_t`. This patch changes that by making those internal methods use the enum type. This should address the feedbacks from llvm#111907. Signed-off-by: Med Ismail Bennani <[email protected]>
Generally no, though I kind of think the enumeration shouldn't belong in the API directory to begin with. I see Ismail changed that so it seems like we're on the same page here. |
+1, may be there was a limitation explaining why it was put in the API tree but I didn't bump into any issue in #111929. May be @adrian-prantl has some more context with regard to that ? |
This patch moves `SBLanguages.h` out of the API tree. This file gets generated at build time using DWARF table-gen file and contains an enumeration of the DWARF supported source language names and there respective code/identifier. Since this is an enum, this shouldn't be part of the API tree but rather it should be included in the `lldb-enumeration` header. Also, that enum was used in internal methods in `lldb_private` by down-casting the enum value type to an `uint16_t`. This patch changes that by making those internal methods use the enum type. This should address the feedbacks from llvm#111907. Signed-off-by: Med Ismail Bennani <[email protected]>
This patch moves `SBLanguages.h` out of the API tree. This file gets generated at build time using DWARF table-gen file and contains an enumeration of the DWARF supported source language names and there respective code/identifier. Since this is an enum, this shouldn't be part of the API tree but rather it should be included in the `lldb-enumeration` header. Also, that enum was used in internal methods in `lldb_private` by down-casting the enum value type to an `uint16_t`. This patch changes that by making those internal methods use the enum type. This should address the feedbacks from llvm#111907. Signed-off-by: Med Ismail Bennani <[email protected]>
This adds an include for SBLanguages.h in lldb-enumerations.h so that files that need this enum do not have to explicitly include SBLanguages.
…#111907)" Temporarily Revert until Chelsea can look at this. With a clean build, SBLanguages.h won't be generated in the build directory at the point when it is included by lldb-enumerations when compiling e.g. Broadcaster.cpp. On a clean build (no pre-existing build directory), the dependency ordering is not explicitly stated so the build will fail. An incremental build will succeed. This reverts commit b355426.
This adds an include for SBLanguages.h in lldb-enumerations.h so that files that need this enum do not have to explicitly include SBLanguages.