[PATCH] D43217: [LLD] Implement /guard:[no]longjmp

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking pretty good. A few minor comments.



================
Comment at: lld/COFF/Config.h:75
 
+enum GuardCFLevel : unsigned {
+  CFG_Off = 0,
----------------
Since we are using enum class above, I'd use enum class too.


================
Comment at: lld/COFF/DriverUtils.cpp:135
+  for (StringRef Arg : SplitArgs) {
+    if (Arg.equals_lower("no"))
+      Config->GuardCF = CFG_Off;
----------------
I'd write these if-else-ifs in the same order as the enum values. I.e. no, nolongjmp and full.


================
Comment at: lld/COFF/DriverUtils.cpp:137-139
+    else if (Arg.equals_lower("cf"))
+      Config->GuardCF = CFG_Full;
+    else if (Arg.equals_lower("longjmp"))
----------------
I'd combine the two `else if`s like `Args.equals_lower() || Args.equals_lower()`


================
Comment at: lld/COFF/Writer.cpp:925
+  // Add the longjmp target table unless the user told us not to.
+  if (Config->GuardCF > CFG_NoLongJmp)
+    maybeAddRVATable(RData, std::move(LongJmpTargets), "__guard_longjmp_table",
----------------
This is the same as `Config->GuardCF == Full`?


https://reviews.llvm.org/D43217