[libcxx-commits] [PATCH] D99789: [libc++] Add fallback standard flags and normalize corresponding feature.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete11.pass.cpp:14
 
-// UNSUPPORTED: c++14, c++17, c++2a
+// UNSUPPORTED: c++14, c++17, c++20
 // UNSUPPORTED: sanitizer-new-delete
----------------
Quuxplusone wrote:
> Here and above, it looks like the intent is "`REQUIRES: c++03 OR c++11`". I don't know if there's any way to express that in llvm-lit. However, I predict that these tests fail in `c++2b` mode; am I right?
> It might be better to duplicate these files into `sized_delete11.pass.cpp` and `sized_delete03.pass.cpp`, add an appropriate `REQUIRES:` to each of them, and then never touch them again.
Nice! I did not know `||` would work here. :)


================
Comment at: libcxx/utils/libcxx/test/params.py:11-12
 
-_allStandards = ['c++03', 'c++11', 'c++14', 'c++17', 'c++2a', 'c++2b']
+_allStandards = {'c++03': None, 'c++11': None, 'c++14': None,
+                 'c++17': 'c++1z', 'c++20': 'c++2a', 'c++2b': None}
 _warningFlags = [
----------------
I like this. Is it worth adding `c++0x`, `c++1y` as fallbacks for those two? I assume the logic of leaving them out is simply that we don't officially support any compiler old enough to lack a `c++14` mode. But it might be useful-to-someone and/or encyclopedic to keep them in the script, I dunno.


================
Comment at: libcxx/utils/libcxx/test/params.py:58-62
+  Parameter(name='std', choices=_allStandardsAndFallbacks, type=str,
             help="The version of the standard to compile the test suite with.",
-            default=lambda cfg: next(s for s in reversed(_allStandards) if hasCompileFlag(cfg, '-std='+s)),
+            default=lambda cfg: getLatestStdFlag(cfg),
             actions=lambda std: [
+              AddFeature(_undoStandardFallback[std] if std in _undoStandardFallback else std),
----------------
ldionne wrote:
> curdeius wrote:
> > curdeius wrote:
> > > curdeius wrote:
> > > > ldionne wrote:
> > > > > 
> > > > Unless I'm mistaken, we still need to call `AddFeature`, no? @ldionne 
> > > Also, in actions, I don't have `cfg`, only `std`...
> > WDYT about:
> > ```
> > AddFeature(next((s for s, f in reversed(_allStandards) if f == std), std)),
> > ```
> > ?
> Yeah, it seems I really botched my code change suggestion :-).
> 
> Basically anything that works is fine by me, but I found the logic with multiple variables at the beginning of the file to be hard to follow.
I think all we're actually doing here is saying, "If the user gave us `c++2a`, turn it into `c++20`." Right? So that would be, like,

    def what_were_doing(flag):
      if flag in _allStandards.values():
        AddFeature(next(s for s, f in _allStandards.items() if f == flag))
      else:
        AddFeature(flag)

which is like... exactly what you wrote, except without the `reversed`, right? The `reversed` would make a difference only if `_allStandards.items()` were in some relevant order to begin with.

I'm not thrilled by the readability of this expression, but I have no concrete improvements to suggest.

So I think I'm just asking to remove the `reversed(`...`)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99789