labath added a comment.

I think this is great. Just a bit of polishing...



================
Comment at: lldb/include/lldb/Core/PluginManager.h:25-29
 #define LLDB_PLUGIN(PluginName)                                                
\
   namespace lldb_private {                                                     
\
   void lldb_initialize_##PluginName() { PluginName::Initialize(); }            
\
   void lldb_terminate_##PluginName() { PluginName::Terminate(); }              
\
   }
----------------
This macro should forward to the "ADV" (advanced?) one.


================
Comment at: lldb/source/API/SystemInitializerFull.cpp:27-28
 
-LLDB_PLUGIN_DECLARE(ABIAArch64);
-LLDB_PLUGIN_DECLARE(ABIARM);
-LLDB_PLUGIN_DECLARE(ABISysV_arc);
-LLDB_PLUGIN_DECLARE(ABISysV_hexagon);
-LLDB_PLUGIN_DECLARE(ABIMips);
-LLDB_PLUGIN_DECLARE(ABIPowerPC);
-LLDB_PLUGIN_DECLARE(ABISysV_s390x);
-LLDB_PLUGIN_DECLARE(ABIX86);
-LLDB_PLUGIN_DECLARE(ObjectFileBreakpad);
-LLDB_PLUGIN_DECLARE(ObjectFileELF);
-LLDB_PLUGIN_DECLARE(ObjectFileMachO);
-LLDB_PLUGIN_DECLARE(ObjectFilePECOFF);
-LLDB_PLUGIN_DECLARE(ObjectFileWasm);
-LLDB_PLUGIN_DECLARE(ObjectContainerBSDArchive);
-LLDB_PLUGIN_DECLARE(ObjectContainerUniversalMachO);
-LLDB_PLUGIN_DECLARE(ScriptInterpreterNone);
-#if LLDB_ENABLE_PYTHON
-LLDB_PLUGIN_DECLARE(OperatingSystemPython)
-LLDB_PLUGIN_DECLARE(ScriptInterpreterPython)
-#endif
-#if LLDB_ENABLE_LUA
-LLDB_PLUGIN_DECLARE(ScriptInterpreterLua)
-#endif
-LLDB_PLUGIN_DECLARE(PlatformFreeBSD)
-LLDB_PLUGIN_DECLARE(PlatformLinux)
-LLDB_PLUGIN_DECLARE(PlatformNetBSD)
-LLDB_PLUGIN_DECLARE(PlatformOpenBSD)
-LLDB_PLUGIN_DECLARE(PlatformWindows)
-LLDB_PLUGIN_DECLARE(PlatformAndroid)
-LLDB_PLUGIN_DECLARE(PlatformMacOSX)
-LLDB_PLUGIN_DECLARE(TypeSystemClang)
-LLDB_PLUGIN_DECLARE(ArchitectureArm)
-LLDB_PLUGIN_DECLARE(ArchitectureMips)
-LLDB_PLUGIN_DECLARE(ArchitecturePPC64)
-LLDB_PLUGIN_DECLARE(DisassemblerLLVMC)
-LLDB_PLUGIN_DECLARE(JITLoaderGDB)
-LLDB_PLUGIN_DECLARE(ProcessElfCore)
-LLDB_PLUGIN_DECLARE(ProcessMachCore)
-LLDB_PLUGIN_DECLARE(ProcessMinidump)
-LLDB_PLUGIN_DECLARE(MemoryHistoryASan)
-LLDB_PLUGIN_DECLARE(InstrumentationRuntimeASan)
-LLDB_PLUGIN_DECLARE(InstrumentationRuntimeTSan)
-LLDB_PLUGIN_DECLARE(InstrumentationRuntimeUBSan)
-LLDB_PLUGIN_DECLARE(InstrumentationRuntimeMainThreadChecker)
-LLDB_PLUGIN_DECLARE(SymbolVendorELF)
-LLDB_PLUGIN_DECLARE(SymbolFileBreakpad)
-LLDB_PLUGIN_DECLARE(SymbolFileDWARF)
-LLDB_PLUGIN_DECLARE(SymbolFilePDB)
-LLDB_PLUGIN_DECLARE(SymbolFileSymtab)
-LLDB_PLUGIN_DECLARE(SymbolVendorWasm)
-LLDB_PLUGIN_DECLARE(UnwindAssemblyInstEmulation)
-LLDB_PLUGIN_DECLARE(UnwindAssembly_x86)
-LLDB_PLUGIN_DECLARE(EmulateInstructionARM)
-LLDB_PLUGIN_DECLARE(EmulateInstructionARM64)
-LLDB_PLUGIN_DECLARE(EmulateInstructionMIPS)
-LLDB_PLUGIN_DECLARE(EmulateInstructionMIPS64)
-LLDB_PLUGIN_DECLARE(EmulateInstructionPPC64)
-LLDB_PLUGIN_DECLARE(ItaniumABILanguageRuntime)
-LLDB_PLUGIN_DECLARE(AppleObjCRuntime)
-LLDB_PLUGIN_DECLARE(SystemRuntimeMacOSX)
-LLDB_PLUGIN_DECLARE(RenderScriptRuntime)
-LLDB_PLUGIN_DECLARE(CPlusPlusLanguage)
-LLDB_PLUGIN_DECLARE(ObjCLanguage)
-LLDB_PLUGIN_DECLARE(ObjCPlusPlusLanguage)
-#if defined(_WIN32)
-LLDB_PLUGIN_DECLARE(ProcessWindows)
-#endif
-#if defined(__FreeBSD__)
-LLDB_PLUGIN_DECLARE(ProcessFreeBSD)
-#endif
-#if defined(__APPLE__)
-LLDB_PLUGIN_DECLARE(SymbolVendorMacOSX)
-LLDB_PLUGIN_DECLARE(ProcessKDP)
-LLDB_PLUGIN_DECLARE(DynamicLoaderDarwinKernel)
-#endif
-LLDB_PLUGIN_DECLARE(StructuredDataDarwinLog)
-LLDB_PLUGIN_DECLARE(PlatformRemoteGDBServer)
-LLDB_PLUGIN_DECLARE(ProcessGDBRemote)
-LLDB_PLUGIN_DECLARE(DynamicLoaderMacOSXDYLD)
-LLDB_PLUGIN_DECLARE(DynamicLoaderPOSIXDYLD)
-LLDB_PLUGIN_DECLARE(DynamicLoaderStatic)
-LLDB_PLUGIN_DECLARE(DynamicLoaderWindowsDYLD)
+#undef LLDB_PLUGIN
+#define LLDB_PLUGIN(p) LLDB_PLUGIN_DECLARE(p)
+#include "Plugins/Plugins.def"
----------------
I don't like this recycling of the name LLDB_PLUGIN. Maybe rename this to 
LLDB_PLUGIN_OP, or the "other" macro to LLDB_PLUGIN_DEFINE ?


================
Comment at: lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp:58
 
-LLDB_PLUGIN(ABISysV_arc)
+LLDB_PLUGIN(ABISysV_arc, ABIARC)
 
----------------
missing _ADV. You might want to double check that the build with all targets 
enabled still works.


================
Comment at: lldb/source/Plugins/Plugins.def.in:12
+|* Clients of this file should define the LLDB_PLUGIN macro to be a           
*|
+|* function-like macro with a single parameter (the name of the plugin)       
*|
+|* including this file will then enumerate all of the targets.                
*|
----------------
Maybe also mention `LLDB_SCRIPT_PLUGIN` ?


================
Comment at: lldb/tools/lldb-test/CMakeLists.txt:28
+
+target_include_directories(lldb-test PRIVATE ${LLDB_PLUGINS_INCLUDES})
+target_include_directories(lldb-test PRIVATE ${LLDB_SOURCE_DIR}/source)
----------------
It looks like you're including the `def` file as `Plugins/Plugins.def` in 
SystemInitializerFull. Can we do the same thing here and avoid this 
`LLDB_PLUGINS_INCLUDES` business ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73067/new/

https://reviews.llvm.org/D73067



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to