[libcxx-commits] [PATCH] D94530: [libc++] improve feature test macro script

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Seems like a generally good direction to me, FWIW.



================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:903
+  std_tests.append(f'#if TEST_STD_VER < {get_std_nr(std_dialects[1])}\n\n' +
+                   generate_std_test(test_list, std_dialects[0]))
+
----------------
I see no obvious reason for the special case here. Sure we //currently// do it as `#if TEST_STD_VER < 14`, but would anything really break if we did it as `#if TEST_STD_VER == 11` instead? It'd be worth trying to find out.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:916
+  else:
+    if_condition = '> ' + get_std_nr(std_dialects[-2])
+  
----------------
I think this should just be `assert not std_dialects[-1][-1].isnumeric()`, and then simplify accordingly. (That would technologically enforce my earlier opinion that whoever changes `2a` to `20` should go ahead and add `2b` at the same time.)


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:923
+
+  return '\n\n'.join(std_tests)
 
----------------
FWIW, I would recommend copying the style from circa line 800: use `template.format("""...""")` instead of f-strings, and use one big `"""` string with all the arguments collected at the bottom, instead of a bunch of little strings `append`'ed together. (This also solves your question about whether f-strings are allowed, because you'd no longer need to use f-strings.)


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:1027
   table = [["Macro Name", "Value"]]
-  for std in get_std_dialects():
+  for std in get_std_dialects()[1:]:
     table += [["**" + std.replace("c++", "C++ ") + "**", ""]]
----------------
This `[1:]` smells questionable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94530