[libc-commits] [PATCH] D94642: [libc] Use #undef isascii in specific header

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

I am to blame for this. I did consider the approach you are suggesting, and also few other less ideal approaches. What you are proposing is the most appropriate way I would think but I did not like it previously for a few reasons.

1. I felt that instead of introducing a one off pattern, we should have a more uniform pattern. As in, we should have the `undef` in all of the internal headers.
2. Even with that approach, it is still sensitive to the order of include files as you have mentioned. Fixing the order breaks the current include order style (which was inherited from the general LLVM style. <https://llvm.org/docs/CodingStandards.html#include-style>)

Both the above problems are addressable I think but quickly go beyond the scope of the problem that was to be addressed. So, considering the problem is only showing up for Fuchsia today, I proposed to put the `undef` next to the Fuchsia includes so that it is clear why we have them at all.

But, now that you have brought it up, I think we should solve this more thoroughly. What I mean is, while your change LGTM, I would like to eliminate these one-off patterns and put in place a more general set of rules/solutions even if it requires breaking away from the general LLVM style for the libc. What do you think of a plan like this, which is mostly along the ideas you have proposed:

1. For the `undef` in the internal header: It would be nice to fold the `#undef <function name>` inside of a macro like this (which doesn't work and my preprocessor knowledge is not enough to come up with alternates) but may be not important because of helper libraries (see #2 below).

  // This macro is to be used inside of the namespace __llvm_libc
  #define LLVM_LIBC_FUNCTION_DECL(name) \
    #undef name
    decltype(::name) name;



2. We will change the include order rule to:
  1. First come public libc headers
  2. Then are non-libc headers. So, in unittests, the test framework headers come next.
  3. Next are libc-internal headers. We will need to define what are libc-internal headers. All headers in `src/...` are internal headers. But, headers from helper libraries outside of the `src/...` directory should also be considered as internal. We currently only have two such libraries which are header only. All internal headers should `#undef`public libc names used by them.

Each of the above groups should be lexicographically sorted.

3. The enforcement of these rules should be tool driven.

Let me know what you think about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94642