[PATCH] D125777: [llvm-dva] 02 - Driver and documentation

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:23
+#include "llvm/DebugInfo/LogicalView/Core/LVType.h"
+
+namespace llvm {
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Let's include `<set>` and `<string>` to make the file self contained.
> `<string>` already included by `LVObject.h`. See previous change.
> `<set>` already included by `LVElement`.
> 
> May be I am not understanding your point. Do you prefer those includes to be here instead of being include indirectly?
Yes, the idea was to make the header more independent from all the other ones including the `LVElement.h` and `LVObject` ones but if this makes no sense (the header defined things that depends on the defined in `LVElement.h` things and those things cannot be just forward declared) and the `<set>`, `<string>` and the other headers are already included there, there is no reason to include them directly.

I've added numerous similar comments for the other headers in next patches, you can just ignore them if they also make no sense. Sorry.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:300-303
+  LVOptions() = default;
+  LVOptions(const LVOptions &) = default;
+  LVOptions &operator=(LVOptions const &) = default;
+  ~LVOptions() = default;
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Just for interest, why do you explicitly define these default members?
> Just to explicitly state that these members use the default implementation.
Thank you for the answer. I see no problem with such declarations.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:19
+#include "llvm/DebugInfo/LogicalView/Core/LVSort.h"
+
+namespace llvm {
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Let's include <set> to make the file self contained.
> `<set>` already included by `LVElement`.
> 
> May be I am not understanding your point. Do you prefer this include to be here instead of being include indirectly?
Yes, the idea was to make the header more independent from all the other ones including the `LVElement.h` one but if this makes no sense (the header defined things that depends on the defined in `LVElement.h` things and those things cannot be just forward declared) and the `<set>` and the other headers are already included there, there is no reason to include them directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125777