sconstab added inline comments.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:318
+ }
+ auto *VertexArray = new Node[VertexSize + 1 /* terminator node */];
+ auto *EdgeArray = new Edge[EdgeSize];
----------------
mattdr wrote:
> As a general rule `new` is a code-smell in modern C++. This should be a
> `vector`.
@mattdr I do agree with the general rule. I also think that in this case where
the structure is immutable, std::vector is wasteful because it needs to keep
separate values for the current number of elements and the current capacity. At
local scope within a function the unneeded value would likely be optimized
away, but then there would be an awkward handoff to transfer the data from the
vector to the array members.
I would not want to see the array members changed to vectors, unless LLVM
provides an encapsulated array structure that does not need to grow and shrink.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:335
+ VertexArray[VI].__edges = EdgeArray + EdgeSize; // terminator node
+ return new GraphT{VertexArray, VertexSize, EdgeArray, EdgeSize,
+ std::forward<ArgT>(Args)...};
----------------
mattdr wrote:
> this should return a `unique_ptr` to signal ownership transfer
Yes, agree.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75936/new/
https://reviews.llvm.org/D75936
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
