[libcxx-commits] [PATCH] D78381: [libc++] Create a small DSL for defining Lit features and parameters

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:51
+  """
+  command = "echo | %{{cxx}} -xc++ -Werror -fsyntax-only %{{compile_flags}} {} -".format(flag)
+  command = _expandSubstitutions(config, command)
----------------
EricWF wrote:
> EricWF wrote:
> > I don't know that this is correct with GCC.
> > And it doesn't handle `-Wno-foo` flags.
> > 
> > IIRC the compiler may only report unknown flags as error if another error occurs during compilation.
> Also, just use `os.devnul` as the input file, because pipes don't work with popen.
> I don't know that this is correct with GCC. And it doesn't handle -Wno-foo flags.

It is correct with GCC outside of `-Wno-foo` flags, but you are correct that it doesn't handle `-Wno-foo` flags correctly. As-is, it will report that all `-Wno-foo` flags are supported on GCC. I went ahead and filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94745 because I thought it was a GCC bug, but Jonathan Wakely nicely explained to me how this was by design. He also suggested using `g++ -Q --help=warnings` (we should have that for Clang!).

I was going to go ahead and implement the `g++ -Q --help=warnings` solution, but then it occurred to me that if GCC reports they "support" a `-Wno-foo` flag when they don't (and they simply ignore it), then why is it a problem for the test suite to add that warning flag? In other words, why would we want to *not* add a flag that gets ignored? I believe that behavior is more in line with their intent when they made that design choice.

For this reason, I'll suggest we leave it as-is, but if you feel strongly about it, please explain and I can use `g++ -Q --help=warnings` on GCC to check for supported `-Wno-foo` flags.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:70
+  output = _subprocess_check_output(command).strip()
+  return locale in output
+
----------------
EricWF wrote:
> Last I checked locales didn't always have a single canonical spelling. Are you handling this case?
This will be handled through trying various different locale names when setting up each locale's feature. This will be in an upcoming patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78381