[PATCH] D33880: COFF: Introduce LD shim around LINK

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: MinGW/Driver.cpp:53
+// Create table mapping all options defined in Options.td
+static const opt::OptTable::Info infoTable[] = {
+#define OPTION(X1, X2, ID, KIND, GROUP, ALIAS, X7, X8, X9, X10, X11, X12)      \
----------------
martell wrote:
> ruiu wrote:
> > infoTable -> InfoTable
> > 
> > (Yet another style issue, and honestly I'm a bit tired of pointing them out, so please please fix them before sending your patch to review. If you do not pay attention to the documented and obvious coding style rules, I would probably suspend doing "real" code review and instead pointing out only style issues until your code follows the coding style. I understand that we cannot and do not want to reduce the error rate to zero, and I really appreciate your work on lld/mingw, but pointing out that variables are in CamelCase in the LLVM coding style on virtually every patch update is a waste of both your and my time.)
> I addressed ALL the CamelCase variables in the patch and left a note for this one specifically before you reviewed it again. You should see a comment a few lines below where `infoTable` is used with the following text
> `infoTable doesn't seem to follow the variable naming guide but this is done consistently across LLVM`
> 
> Here are references to the locations I found using this exact spelling which is why I left it.
> 
> LLD
> 
> COFF/DriverUtils.cpp
> lib/Driver/DarwinLdDriver.cpp
> 
> LLVM
> 
> tools/llvm-mt/llvm-mt.cpp
> tools/llvm-rc/llvm-rc.cpp
> lib/ToolDrivers/llvm-lib/LibDriver.cpp
> tools/llvm-cvtres/llvm-cvtres.cpp
> unittests/Option/OptionParsingTest.cpp
> etc
> 
> Do you still want me to change it?
I understand you frustration Rui, I truly do (especially given earlier revisions of this and other patches), but this one was surely clearly pointed out by Martell before - unfortunately I guess the comment was buried in the log and not at the right spot in the inline diff.

If we give Martell the benefit of doubt that it is ok wrt to variable naming (since he did point this one out explicitly before with a reasoning) and other style issues I assume he's fixed with clang-format by now, what's the status of functional review of this patch? Is it almost done with a few minor issues yet to be found, or has that not been done thoroughly yet at all due to major style issues in all the previous iterations?


Repository:
  rL LLVM

https://reviews.llvm.org/D33880