mattdr added a comment. Adding a few early style notes for the next round, but overall echo @chandlerc that this seems significantly outside of normal LLVM code.
================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:26 + +#ifndef IMMUTABLEGRAPH_H +#define IMMUTABLEGRAPH_H ---------------- It's sort of surprising that the LLVM style guide doesn't call this out explicitly, but `#include` guards are supposed to include the full file path. If they just used the filename, like, this, files with the same name in different paths would collide. For an example of the expected style, see an adjacent header in this directory: https://github.com/llvm/llvm-project/blob/ba8b3052b59ebee4311d10bee5209dac8747acea/llvm/lib/Target/X86/X86AsmPrinter.h#L10 ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:318 + } + auto *VertexArray = new Node[VertexSize + 1 /* terminator node */]; + auto *EdgeArray = new Edge[EdgeSize]; ---------------- As a general rule `new` is a code-smell in modern C++. This should be a `vector`. ================ 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)...}; ---------------- this should return a `unique_ptr` to signal ownership transfer 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
