DavidSpickett added a comment.

LGTM with /// for the comments in the test.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1525
 
+void AddAAPCSVolatileBitfieldArgs(const ArgList &Args, ArgStringList &CmdArgs) 
{
+  if (!Args.hasFlag(options::OPT_faapcs_bitfield_width,
----------------
stuij wrote:
> DavidSpickett wrote:
> > Name this AddARMAAPCSVolatileBitfieldArgs to match the other helpers.
> I deliberately didn't add `ARM` to the name as this function is used by both 
> the Arm and AArch64 targets, and `ARM` would cause confusion. I figured AAPCS 
> is differentiating enough, as it implies Arm without choosing a specific arch.
Good point. At least one of the A's stands for Arm in any case.


================
Comment at: clang/test/Driver/arm-aarch64-bitfield-flags.c:2
+// check -faapcs-bitfield-width/-fno-aapcs-bitfield-width
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-m.main 
-faapcs-bitfield-width -### %s 2>&1 | FileCheck 
--check-prefixes=WIDTH,INVERSE-WIDTH %s
+// RUN: %clang -target aarch64-arm-none-eabi -march=armv8-m.main 
-faapcs-bitfield-width -### %s 2>&1 | FileCheck 
--check-prefixes=WIDTH,INVERSE-WIDTH %s
----------------
Nit: Use /// for comments that aren't run/check lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96784

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

Reply via email to