[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133906#3792666 , @JDevlieghere 
wrote:

> In D133906#3792230 , @labath wrote:
>
>>   // no .def file
>>   #define LLDB_FORWARD_CLASS(cls) \
>> namespace lldb_private { class cls; } \
>> namespace lldb {  using cls##SP = std::shared_ptr; } \
>> ...
>>   
>>   LLDB_FORWARD_CLASS(Foo)
>>   LLDB_FORWARD_CLASS(Bar)
>>   ...
>
> Works for me, but I don't see how that would help with go-to definition. 
> Xcode still won't show you the macro expansion so there's nothing to click 
> through, which was Jim's complaint.

Right, I see. I misunderstood the problem somehow. That said, I can imagine 
Xcode offering a (clickable) tooltip showin the macro expansion in this case. I 
know of an IDE that does that, and it's pretty cool.

In D133906#3793505 , @clayborg wrote:

> Wouldn't this also slow down compilation a bit? Each file that #include 
> "lldb-forward.h" will not do a large amount of preprocessor stuff for each 
> and every file that is compiled that includes this?

Technically yes, but I very much doubt the difference will be measurable. The 
slowness of compiling c++ comes from instantiating templates and other fancy 
stuff. The preprocessor step is ridiculously fast.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I really wish the .def files would actually generate a header file in the 
output directory. I am not a fan of using code navigation when saying "take me 
to the definition of "class Block;" and it shows:

  #define LLDB_FORWARD_CLASS(Name) class Name;
  #include "lldb-forward.def"
  #undef LLDB_FORWARD_CLASS

Also as Jim pointed out, we now will generate shared, unique, and weak pointers 
for everything, even if those classes  are never used that way.

I am not a fan of this. Wouldn't this also slow down compilation a bit? Each 
file that #include "lldb-forward.h" will not do a large amount of preprocessor 
stuff for each and every file that is compiled that includes this?


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

To some extent, the fact that we define an XxxSP for a class is saying "This is 
a thing we mostly pass around as a shared pointer" which would be lost if every 
time we make an SP or UP we use the same syntax.  But I'm not sure that that's 
a terribly strong objection.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

SP and UP templates seems fine to me.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D133906#3792230 , @labath wrote:

> In D133906#3791352 , @JDevlieghere 
> wrote:
>
>> In D133906#3791153 , @jingham 
>> wrote:
>>
>>> This patch makes me a little sad because it breaks the "Jump to Definition" 
>>> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
>>> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
>>> definition on the class name in the shared pointer definition to jump to 
>>> the class.  Now the first jump takes you to:
>>>
>>> LLDB_FORWARD_CLASS(Process)
>>>
>>> in the lldb-forward.def file, and you can't go any further because the IDE 
>>> can't tell what to do with the contents of the .def file (it has no way to 
>>> know how it was imported to create real definitions).  These .def 
>>> insertions almost always make things harder to find in the actual code, so 
>>> they aren't free.
>>
>> The alternative would be to have tablegen generate the header, but I think 
>> that's overkill, and I'm not even sure Xcode would pick the generated header.
>
> I have a feeling that the code would be simpler if you reversed the 
> "iteration order", and it might even make the go-to definition command more 
> useful. I'm thinking of something like
>
>   // no .def file
>   #define LLDB_FORWARD_CLASS(cls) \
> namespace lldb_private { class cls; } \
> namespace lldb {  using cls##SP = std::shared_ptr; } \
> ...
>   
>   LLDB_FORWARD_CLASS(Foo)
>   LLDB_FORWARD_CLASS(Bar)
>   ...

Works for me, but I don't see how that would help with go-to definition. Xcode 
still won't show you the macro expansion so there's nothing to click through, 
which was Jim's complaint.

> That said, my preferred solution would be something like `template T> using SP = std::shared_ptr`, and then replacing all `XyzSP` with 
> `SP`. The two main benefits (besides simplifying the forward file) I see 
> are:
>
> - being able to write `SP`. With the current pattern, you'd have 
> to introduce a whole new class of typedefs, or live with the fact that 
> `shared_ptr` looks very different from XyzSP
> - being compatible with the llvm naming scheme. XyzSP and xyz_sp would 
> collide if they both used camel case. With this, they won't because one of 
> them will not exist.

This would address Jim's concern, but it's more churn. If everyone's okay with 
this approach I'm happy to do the mechanical find-and-replace.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133906#3791352 , @JDevlieghere 
wrote:

> In D133906#3791153 , @jingham wrote:
>
>> This patch makes me a little sad because it breaks the "Jump to Definition" 
>> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
>> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
>> definition on the class name in the shared pointer definition to jump to the 
>> class.  Now the first jump takes you to:
>>
>> LLDB_FORWARD_CLASS(Process)
>>
>> in the lldb-forward.def file, and you can't go any further because the IDE 
>> can't tell what to do with the contents of the .def file (it has no way to 
>> know how it was imported to create real definitions).  These .def insertions 
>> almost always make things harder to find in the actual code, so they aren't 
>> free.
>
> The alternative would be to have tablegen generate the header, but I think 
> that's overkill, and I'm not even sure Xcode would pick the generated header.

I have a feeling that the code would be simpler if you reversed the "iteration 
order", and it might even make the go-to definition command more useful. I'm 
thinking of something like

  // no .def file
  #define LLDB_FORWARD_CLASS(cls) \
namespace lldb_private { class cls; } \
namespace lldb {  using cls##SP = std::shared_ptr; } \
...
  
  LLDB_FORWARD_CLASS(Foo)
  LLDB_FORWARD_CLASS(Bar)
  ...

That said, my preferred solution would be something like `template 
using SP = std::shared_ptr`, and then replacing all `XyzSP` with `SP`. 
The two main benefits (besides simplifying the forward file) I see are:

- being able to write `SP`. With the current pattern, you'd have to 
introduce a whole new class of typedefs, or live with the fact that 
`shared_ptr` looks very different from XyzSP
- being compatible with the llvm naming scheme. XyzSP and xyz_sp would collide 
if they both used camel case. With this, they won't because one of them will 
not exist.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D133906#3791153 , @jingham wrote:

> This patch makes me a little sad because it breaks the "Jump to Definition" 
> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
> definition on the class name in the shared pointer definition to jump to the 
> class.  Now the first jump takes you to:
>
> LLDB_FORWARD_CLASS(Process)
>
> in the lldb-forward.def file, and you can't go any further because the IDE 
> can't tell what to do with the contents of the .def file (it has no way to 
> know how it was imported to create real definitions).  These .def insertions 
> almost always make things harder to find in the actual code, so they aren't 
> free.

The alternative would be to have tablegen generate the header, but I think 
that's overkill, and I'm not even sure Xcode would pick the generated header.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.

In D133906#3791153 , @jingham wrote:

> This patch makes me a little sad because it breaks the "Jump to Definition" 
> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
> definition on the class name in the shared pointer definition to jump to the 
> class.  Now the first jump takes you to:
>
> LLDB_FORWARD_CLASS(Process)
>
> in the lldb-forward.def file, and you can't go any further because the IDE 
> can't tell what to do with the contents of the .def file (it has no way to 
> know how it was imported to create real definitions).  These .def insertions 
> almost always make things harder to find in the actual code, so they aren't 
> free.

@jingham to be hosted, as a fellow Xcode user, this indirection annoyed me more 
than I found it convenient. May be we should have Xcode go directly to the 
template argument class when jumping to definition on smart pointers.

@jdevlieghere LGTM! Thanks!


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D133906#3791153 , @jingham wrote:

> This patch makes me a little sad because it breaks the "Jump to Definition" 
> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
> definition on the class name in the shared pointer definition to jump to the 
> class.  Now the first jump takes you to:
>
> LLDB_FORWARD_CLASS(Process)
>
> in the lldb-forward.def file, and you can't go any further because the IDE 
> can't tell what to do with the contents of the .def file (it has no way to 
> know how it was imported to create real definitions).  These .def insertions 
> almost always make things harder to find in the actual code, so they aren't 
> free.

@jingham to be hosted, as a fellow Xcode user, this indirection annoyed me more 
than I found it convenient. May be we should have Xcode go directly to the 
template argument class when jumping to definition on smart pointers.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This patch makes me a little sad because it breaks the "Jump to Definition" 
chain Xcode (and I bet it does in other IDE's that support this.)  You used to 
be able to do "Jump to Definition" on ProcessSP, then jump to definition on the 
class name in the shared pointer definition to jump to the class.  Not the 
first jump takes you to:

LLDB_FORWARD_CLASS(Process)

in the lldb-forward.def file, and you can't go any further.  These .def 
insertions almost always make things harder to find in the actual code, so they 
aren't free.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Definitely better than the previous contents!


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: mib, aprantl, labath.
Herald added a project: All.
JDevlieghere requested review of this revision.

Generate lldb-forward with .def file. The benefit is that we don't have to keep 
the list of classes, and typedefs in sync.


https://reviews.llvm.org/D133906

Files:
  lldb/include/lldb/lldb-forward.def
  lldb/include/lldb/lldb-forward.h

Index: lldb/include/lldb/lldb-forward.h
===
--- lldb/include/lldb/lldb-forward.h
+++ lldb/include/lldb/lldb-forward.h
@@ -16,276 +16,14 @@
 // lldb forward declarations
 namespace lldb_private {
 
-class ABI;
-class ASTResultSynthesizer;
-class ASTStructExtractor;
-class Address;
-class AddressRange;
-class AddressResolver;
-class ArchSpec;
-class Architecture;
-class Args;
-class ArmUnwindInfo;
-class Baton;
-class Block;
-class Breakpoint;
-class BreakpointID;
-class BreakpointIDList;
-class BreakpointList;
-class BreakpointLocation;
-class BreakpointLocationCollection;
-class BreakpointLocationList;
-class BreakpointName;
-class BreakpointOptionGroup;
-class BreakpointOptions;
-class BreakpointPrecondition;
-class BreakpointResolver;
-class BreakpointSite;
-class BreakpointSiteList;
-class BroadcastEventSpec;
-class Broadcaster;
-class BroadcasterManager;
-class CXXSyntheticChildren;
-struct CacheSignature;
-class CallFrameInfo;
-class CommandInterpreter;
-class CommandInterpreterRunOptions;
-class CommandObject;
-class CommandObjectMultiword;
-class CommandReturnObject;
-class Communication;
-class CompactUnwindInfo;
-class CompileUnit;
-class CompilerDecl;
-class CompilerDeclContext;
-class CompilerType;
-class Connection;
-class ConnectionFileDescriptor;
-class ConstString;
-class ConstStringTable;
-class DWARFCallFrameInfo;
-class DWARFDataExtractor;
-class DWARFExpression;
-class DWARFExpressionList;
-class DataBuffer;
-class WritableDataBuffer;
-class DataBufferHeap;
-class DataEncoder;
-class DataExtractor;
-class DataFileCache;
-class Debugger;
-class Declaration;
-class DiagnosticManager;
-class Disassembler;
-class DumpValueObjectOptions;
-class DynamicCheckerFunctions;
-class DynamicLoader;
-class Editline;
-class EmulateInstruction;
-class Environment;
-class EvaluateExpressionOptions;
-class Event;
-class EventData;
-class EventDataStructuredData;
-class ExecutionContext;
-class ExecutionContextRef;
-class ExecutionContextScope;
-class Expression;
-class ExpressionTypeSystemHelper;
-class ExpressionVariable;
-class ExpressionVariableList;
-class File;
-class FileSpec;
-class FileSpecList;
-class Flags;
-class FormatManager;
-class FormattersMatchCandidate;
-class FuncUnwinders;
-class Function;
-class FunctionCaller;
-class FunctionInfo;
-class IOHandler;
-class IOObject;
-class IRExecutionUnit;
-class InlineFunctionInfo;
-class Instruction;
-class InstructionList;
-class InstrumentationRuntime;
-class JITLoader;
-class JITLoaderList;
-class Language;
-class LanguageCategory;
-class LanguageRuntime;
-class LineTable;
-class Listener;
-class Log;
-class Mangled;
-class Materializer;
-class MemoryHistory;
-class MemoryRegionInfo;
-class MemoryRegionInfos;
-class Module;
-class ModuleList;
-class ModuleSpec;
-class ModuleSpecList;
-class ObjectContainer;
-class ObjectFile;
-class ObjectFileJITDelegate;
-class OperatingSystem;
-class OptionGroup;
-class OptionGroupOptions;
-class OptionGroupPlatform;
-class OptionValue;
-class OptionValueArch;
-class OptionValueArgs;
-class OptionValueArray;
-class OptionValueBoolean;
-class OptionValueChar;
-class OptionValueDictionary;
-class OptionValueEnumeration;
-class OptionValueFileSpec;
-class OptionValueFileSpecList;
-class OptionValueFormat;
-class OptionValueFormatEntity;
-class OptionValueLanguage;
-class OptionValuePathMappings;
-class OptionValueProperties;
-class OptionValueRegex;
-class OptionValueSInt64;
-class OptionValueString;
-class OptionValueUInt64;
-class OptionValueUUID;
-class Options;
-class PathMappingList;
-class PersistentExpressionState;
-class Platform;
-class Process;
-class ProcessAttachInfo;
-class ProcessInfo;
-class ProcessInstanceInfo;
-class ProcessInstanceInfoMatch;
-class ProcessLaunchInfo;
-class ProcessModID;
-class Property;
-class Queue;
-class QueueImpl;
-class QueueItem;
-class REPL;
-class RecognizedStackFrame;
-class RegisterCheckpoint;
-class RegisterContext;
-class RegisterValue;
-class RegularExpression;
-class RichManglingContext;
-class Scalar;
-class ScriptInterpreter;
-class ScriptInterpreterLocker;
-class ScriptedProcessInterface;
-class ScriptedThreadInterface;
-class ScriptedSyntheticChildren;
-class SearchFilter;
-class Section;
-class SectionList;
-class SectionLoadHistory;
-class SectionLoadList;
-class Settings;
-class SourceManager;
-class SourceManagerImpl;
-class StackFrame;
-class StackFrameList;
-class StackFrameRecognizer;
-class StackFrameRecognizerManager;
-class StackID;
-class Status;
-class StopInfo;
-class Stoppoint;
-class