MaskRay added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4858
 
+  if (Arg *A = Args.getLastArg(options::OPT_fbasicblock_sections_EQ)) {
+    CmdArgs.push_back(
----------------
If we want to pass the option verbatim, `A->render(Args, CmdArgs);`

However, we actually want (a convention) to catch the error at the driver 
level. So the value checking in Frontend/CompilerInvocation.cpp should be moved 
here.

(Note that you used `err_drv_invalid_value` in Frontend/CompilerInvocation.cpp 
. drv is short for driver)


================
Comment at: clang/test/CodeGen/basicblock-sections.c:35
+//
+// BB_WORLD: .section .text.world,"ax",@progbits
+// BB_WORLD: world
----------------
I haven't read through the previous threads whether we should use a .c -> .s 
test here. Assume we've decided to do that, `@progbits` should be followed by 
`{{$}}` to ensure there is no unique assembly linkage.


================
Comment at: clang/test/CodeGen/basicblock-sections.c:38
+// BB_WORLD: .section .text.world,"ax",@progbits,unique
+// BB_WORLD: a.BB.world
+// BB_WORLD: .section .text.another,"ax",@progbits
----------------
If `world` is followed by a colon, the colon should be added. The little detail 
will make it clear that this is a label.


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

https://reviews.llvm.org/D68049



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to