[PATCH] D125778: [llvm-dva] 03 - Logical elements
Pavel Samolysov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 24 06:30:43 PDT 2022
psamolysov added a comment.
There a lot of small nit comments and a few questions. These questions are by no means a criticism of your code or design decisions, they are here just to let me understand more about using classes in LLVM Support. Thank you in advance for the answers,
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h:74
+ // String used for printing objects with no line number.
+ virtual std::string noLineAsString(bool ShowZero) const override;
+
----------------
In order to be aligned with the `lineNumberAsString` method. The default value could be set up for the parameter in the other `noLineAsString` methods as well. `virtual` also can be eliminated.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:20
+#include "llvm/DebugInfo/CodeView/TypeIndex.h"
#include "llvm/DebugInfo/LogicalView/Core/LVSupport.h"
+#include <list>
----------------
Could you add `#include <limits>` for `std::numeric_limits<uint64_t>`?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:188
+ }
+ LVObject &operator=(LVObject const &) = delete;
+ virtual ~LVObject() = default;
----------------
We have a copy constructor but no copy assigned operator. Could you explain why? Thank you.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:21
+#include "llvm/Support/ToolOutputFile.h"
+
+namespace llvm {
----------------
What about `#include <map>` to make the header more self-contained?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:68
+protected:
+ LVScopeRoot *Root = nullptr;
+ std::string InputFilename;
----------------
Why `Root` cannot be a smart pointer in order to not use the `new/delete` machinery below?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:109
+ StringRef getFilename() const { return InputFilename; }
+ void setFilename(std::string Name) { InputFilename = Name; }
+ StringRef getFileFormatName() const { return FileFormatName; }
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:121
+ void setCompileUnit(LVScope *Scope) {
+ CompileUnit = static_cast<LVScopeCompileUnit *>(Scope);
+ }
----------------
Are we sure the `Scope` are always a pointer to `LVScopeCompileUnit`? What about to add an assert in the function before the assignment?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:52
+using LVOffsetElementMap = std::map<LVOffset, LVElement *>;
+
----------------
Let's add `#include <map>` to make the header more self-contained and independent of the other included headers.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:88-99
+ LVAutoTypes *Types = nullptr;
+ LVAutoSymbols *Symbols = nullptr;
+ LVAutoScopes *Scopes = nullptr;
+ LVAutoLines *Lines = nullptr;
+
+ // Vector of elements (types, scopes and symbols).
+ // It is the union of (*Types, *Symbols and *Scopes) to be used for
----------------
Just a question. The `LVScope` class is not copyable at all. Why values of these `LVAutoTypes`, `LVAutoSymbols` etc. cannot be class members, not pointers to the values? At least, having the members as values eliminates a lot of `if (Children) ...` checks in the implementation.
If they cannot be just values, maybe it makes sense to use smart pointers here (`std::unique_ptr`?) instead of create the members with plain `new` and delete then in the destructor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:202
+ bool Full = true) const override;
+ void sort();
+
----------------
I believe a comment is required at least to let the class' user understand that this is sort by what.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:211
+ virtual LVScope *getCompileUnitParent() const override {
+ return LVElement::getCompileUnitParent();
+ }
----------------
I'm not sure but is `using LVElement::getCompileUnitParent();` not enough?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:221
+
+ virtual void resolve() override;
+ virtual void resolveName() override;
----------------
Either `virtual` or `override` is enough.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:260
+ }
+ virtual void setReference(LVElement *Element) override {
+ setReference(static_cast<LVScope *>(Element));
----------------
`setReference(LVScope *)` above is not marked as `virtual`, only `override` is used.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:304
+ // Names (files and directories) used by the Compile Unit.
+ std::vector<size_t> Filenames;
+
----------------
Is this a vector of indices?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:476-477
+
+ virtual void resolveExtra() override;
+ virtual void resolveReferences() override;
+
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:33
+public:
+ typedef typename SmallVector<T, N>::iterator iterator;
+ LVAutoSmallVector() : SmallVector<T, N>::SmallVector() {}
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/CMakeLists.txt:33
"${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Core"
+ "${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Readers"
)
----------------
I see no headers in the `Readers` path in the patch. Should the line be moved in a next patch in the series or `LVReader.h` moved in the `Readers` directory?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:431
+void LVElement::printFileIndex(raw_ostream &OS, bool Full) const {
+ if (options().getPrintFormatting() && options().getAttributeAnySource() &&
+ getFilenameIndex()) {
----------------
Early return if the inverted condition takes place is more preferable by the coding style.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:111
+ if (options().getAttributeQualifier()) {
+ // The qualifier include the states information and the source filename
+ // that contains the line element.
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:28
+//===----------------------------------------------------------------------===//
+// Class to represent an split context.
+//===----------------------------------------------------------------------===//
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:90
+
+ // Return error if enable to create a split context location.
+ if (Error Err = SplitContext.createSplitFolder(SplitFolder))
----------------
if unable?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:115
+// information contained in the binary file specified in the command line.
+// This is the main entry point for the Reader and perform the following
+// steps:
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:110-111
+void LVScope::addElement(LVLine *Line) {
+ assert(!Line->getParent() && "Line already inserted");
+ assert(Line && "Invalid line.");
+ if (!Lines)
----------------
It definitely makes sense to swap these two lines.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:134-135
+void LVScope::addElement(LVScope *Scope) {
+ assert(!Scope->getParent() && "Scope already inserted");
+ assert(Scope && "Invalid scope.");
+ if (!Scopes)
----------------
It definitely makes sense to swap these two lines.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:161-162
+void LVScope::addElement(LVSymbol *Symbol) {
+ assert(!Symbol->getParent() && "Symbol already inserted");
+ assert(Symbol && "Invalid symbol.");
+ if (!Symbols)
----------------
It definitely makes sense to swap these two lines.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:188-189
+void LVScope::addElement(LVType *Type) {
+ assert(!Type->getParent() && "Type already inserted");
+ assert(Type && "Invalid type.");
+ if (!Types)
----------------
It definitely makes sense to swap these two lines.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:214
+bool LVScope::removeElement(LVElement *Element) {
+ auto predicate = [&](LVElement *Item) -> bool { return Item == Element; };
+ auto removeElement = [&](auto &Container) -> bool {
----------------
`Element` is just a pointer, it can be captured by value.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:215
+ auto predicate = [&](LVElement *Item) -> bool { return Item == Element; };
+ auto removeElement = [&](auto &Container) -> bool {
+ auto Iter = std::remove_if(Container->begin(), Container->end(), predicate);
----------------
Only captured variables are `Element` that is a pointer and `predicate` that is a lambda with a very short capture list. Both can be captured by value but by reference is ok too.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:254-255
+ LVSymbols References;
+ References.insert(References.end(), ReferenceSymbols->begin(),
+ ReferenceSymbols->end());
+
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:260
+ std::remove_if(Symbols.begin(), Symbols.end(),
+ [&](LVSymbol *Item) -> bool { return Item == Symbol; });
+ if (Iter != Symbols.end())
----------------
`Symbol` is just a pointer and can be captured by value.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:461
+ QualifiedName.append("::");
+ QualifiedName.append(std::string(getName()));
+}
----------------
Are you sure the explicitly invoked constructor `std::string(getName())` is required? It looks like this converts a `StringRef` into a temporary `std::string`, then makes a copy of the `std::string` and put the copy into the `QualifiedName.append` method. `QualifiedName.append(getName());` should be enough, shouldn't it?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:874
+// Increase the added element counters, for printing summary.
+// Notify the Reader if element comparison.
+void LVScopeCompileUnit::addedElement(LVLine *Line) { increment(Line); }
----------------
There is no notifications are sent below. Should this comment line be added to a next patch in the series?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:902
+ // Collect only unique names.
+ UniqueNames.insert(std::string(Name));
+ }
----------------
It might work.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:904
+ }
+ for (std::string Name : UniqueNames)
+ OS << std::string(Indentation, ' ') << formattedKind(Kind) << " "
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1161
+ LVScope *Reference = getReference();
+ if (Reference->getIsExternal()) {
+ Reference->resetIsExternal();
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1247
+ if (const LVSymbols *Symbols = getSymbols()) {
+ AddComma = false;
+ for (LVSymbol *Symbol : *Symbols)
----------------
`AddComma` has already been initialized, this line looks as not required.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSort.cpp:41
+ const LVObject *RHS) {
+ return std::string(LHS->getName()) < std::string(RHS->getName());
+}
----------------
If `getName()` returns an `llvm::StringRef` then there is the `operator<` for `llvm::StringRef`, so there is no need to create a temporary `std::string` for LHS as well as `RHS`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:254
+ // "std::type<int,std::less<int>,std::allocator<int>,false>"
+ // Instead of the the incomplete names:
+ // "type<float,less,allocator,false>"
----------------
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:264
+ // The argument types always are qualified.
+ Name.append(std::string(getTypeQualifiedName()));
+
----------------
Just to try. `llvm::StringRef` should be convertible to `std::string`. If this works, the other creations of temporary `std::string` in this function will be able to be eliminated too.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:64
+ template <typename T> T *create() {
+ T *Element = new T();
+ EXPECT_NE(Element, nullptr);
----------------
A `std::bad_alloc` will be thrown if the test is out of memory. It makes sense to wrap the line in the `try-catch` block or use the non-throwing overload of the new operator.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125778/new/
https://reviews.llvm.org/D125778
More information about the llvm-commits
mailing list