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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/test/libcxx/selftest/dsl/dsl.sh.py:34
+
+class SetupConfigs(unittest.TestCase):
+    def setUp(self):
----------------
A short docstring for each class would be a nice addition.


================
Comment at: libcxx/test/libcxx/selftest/dsl/dsl.sh.py:72
+        return found[0]
+
+class TestHasCompileFlag(SetupConfigs):
----------------
I think the python style guidlines wants 2 spaces between class definitions.


================
Comment at: libcxx/test/libcxx/selftest/dsl/dsl.sh.py:92
+            dsl.hasLocale(self.config, 'en_US.UTF-8')
+        except Exception:
+            self.fail("checking for hasLocale should not explode")
----------------
Can we watch a more specific exception as to not swallow other errors that may occur?


================
Comment at: libcxx/utils/libcxx/test/dsl.py:51
+  """
+  command = "echo | %{{cxx}} -xc++ -Werror -fsyntax-only %{{compile_flags}} {} -".format(flag)
+  command = _expandSubstitutions(config, command)
----------------
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.


================
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:
> 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.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:70
+  output = _subprocess_check_output(command).strip()
+  return locale in output
+
----------------
Last I checked locales didn't always have a single canonical spelling. Are you handling this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78381