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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:23
+#include "llvm/DebugInfo/LogicalView/Core/LVType.h"
+
+namespace llvm {
----------------
psamolysov wrote:
> 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.
Thanks for the explanation. Based on it, I am happy to include those header files.


================
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;
----------------
psamolysov wrote:
> 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.
Thanks.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:19
+#include "llvm/DebugInfo/LogicalView/Core/LVSort.h"
+
+namespace llvm {
----------------
psamolysov wrote:
> 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.
Thanks for the explanation. Based on it, I am happy to include those header files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125777