peter.smith added a comment. I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. From looking at the source code alone - assuming that most people would not track down the commit message/review for extra context - I found it difficult to work out the convention for when Flags is used and when Tags is used. I've made a suggestion for some comments. This can be applied prior to commit if you want to take it up.
================ Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:202 + Multilib::tag_set &Flags); void addX86AlignBranchArgs(const Driver &D, const llvm::opt::ArgList &Args, ---------------- I can see the reason to keep the name `addMultilibFlag`. At this point is the tag_set expected to be simplified tags or full command line flags. If it is the former I think it would be good to change Flags to Tags here. May also be useul to add a \p for Flags (or Tags) if there are any requirements, or just useful information on what it is expected to be. Parameter name also applies to CommonArgs.cpp below. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145567/new/ https://reviews.llvm.org/D145567 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
