[libcxx-commits] [PATCH] D101293: [libc++] Move <__sso_allocator> out of include/ into src/. NFCI.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/src/CMakeLists.txt:67
   list(APPEND LIBCXX_SOURCES
+    include/locale_sso_allocator.h
     ios.cpp
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > Why is that needed? We normally don't add headers to the list of sources?
> > > I agree that AFAIK from my knowledge of CMake, it should not be needed. However, look at lines 19-21, 32-43, 89-90, 101-104.
> > > I'd be happy to kill off those lines at the same time as this one (in fact I'll update this patch to do so), but if we keep those lines then I think we should keep this one too, because presumably they're all there for the same reason (or non-reason).
> > > 
> > > I see the comment `Add all the headers to the project for IDEs` on line 118. Is that the rationale for adding these .h files to `LIBCXX_SOURCES`?
> > CMake indeed only needs for the IDE. (Unless the header is generated, but that's not the case here.)
> @Mordante: "CMake indeed only needs for the IDE" — So, would you say we should keep all the files in CMakeLists.txt for the benefit of IDE-users? Your choice of the word "...only..." is throwing me a little bit. :)
> 
> I.e. is this part of the current patch (removing 20 lines from this file) a good idea or a bad idea?  (I'm conservatively leaning toward "bad idea".)
As for the "only". CMake doesn't need the header to figure out the dependencies of the `.cpp` files, but CMake can't determine which headers should be part of your IDE project.  You don't want every header (indirectly) included to be part of your project. 

Having the headers in the list doesn't hurt us. So I think removing them is a bad idea, it hurts users using an IDE. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101293