[PATCH] D137489: [LLD][MinGW] Add --error-limit=<N> option

Mon Nov 7 03:03:39 PST 2022

mstorsjo added a reviewer: rnk.
mstorsjo accepted this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

Looks fine to me overall, one minor nit to address, and a little discussion on some details.

================
Comment at: lld/MinGW/Driver.cpp:397
+    if (s.getAsInteger(10, n))
+      error(a->getSpelling() + " number expected, but got " + s);
+    else
----------------
It's somewhat unusual to interpret the argument values that much in the mingw driver (except in the cases where they need to be rewritten), but I see the intent of making the error message mention the right name - so it's probably fine.

(Technically I guess we have that issue for any other argument too, where we just pass values on blindly, and the COFF linker may complain about their values later on.)

================
Comment at: lld/test/MinGW/error-limit.test:12
+RUN: not ld.lld -### foo.o -m i386pep --error-limit=XYZ 2>&1 | FileCheck -check-prefix=WRONG %s
+WRONG:      --error-limit number expected, but got XYZ
+
----------------
I think it'd be nice with a `:` after the option name here; in the case of the COFF driver, the option name itself contains it so there's no need, but e.g. the ELF linker seems to manually add it in some similar cases.

================
Comment at: lld/test/MinGW/error-limit.test:14
+
+RUN: not ld.lld -m i386pep --error-limit=5 01 02 03 04 05 06 07 08 09 10 2>&1 \
+RUN:   | FileCheck -check-prefix=LIMIT5 %s
----------------
I think we don't have any lld mingw driver tests that actually invoke the coff linker, all of them just use `-###` to see what it outputs. Strictly speaking, we tend to avoid such tests, although this one seems harmless (although kinda redundant, as this gets covered in total by the other testcases too)?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137489/new/

https://reviews.llvm.org/D137489