[libcxx-commits] [PATCH] D154246: [libc++] Add check for building with picolibc

[Michael Platings via Phabricator via libcxx-commits]

michaelplatings marked 2 inline comments as done.
michaelplatings added inline comments.


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:17
+    # which for picolibc is defined in picolibc.h.
+    ' -include picolibc.h'
+))
----------------
arichardson wrote:
> Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc? I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?
> 
> As far as I recall from the last time I tried to use libc++ with picolibc this was not needed but I may have been building a slightly different configuration
> Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc?

Most libc++ headers already indirectly include picolibc.h so most of the time this isn't a problem in practice. It's a minority of tests that fail without this include.

> I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?

I agree with you that it would be cleaner to remove this line and instead change the libc++ headers. However I can imagine that adding extra includes could cause undesirable knock-on effects. I think it's best to limit the scope of this change to only adding the test setup and not touching libc++ itself.


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:38
+# using this feature.
+config.available_features.add('LIBCXX-PICOLIBC-FIXME')
+
----------------
arichardson wrote:
> Could this be autodetected instead by checking for the presence of the picolibc header? Then it would also work out of the box with the default config file.
I don't think we can infer that we're targeting picolibc from the presence of the header. It might also happen to be on an include path in a scenario in which another platform is targeted. I think it's best to be explicit here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154246