search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.8k stars 10.98k forks source link

[libc] Migrate to using LIBC_NAMESPACE_DECL for namespace declaration over LIBC_NAMESPACE #97655

Open PiJoules opened 3 days ago

PiJoules commented 3 days ago

97109 introduces LIBC_NAMESPACE_DECL which defines to LIBC_NAMESPACE with a hidden visibility attribute. namespace LIBC_NAMESPACE_DECL { should be used over namespace LIBC_NAMESPACE { when declaring a namespace which will contain internal functions/globals. This ensures anything declared in LIBC_NAMESPACE_DECL will have hidden visibility independent of the build system or global compile flags controlling visibility. Note this won't affect the visibility of public C functions. Those are controlled via LLVM_LIBC_FUNCTION_ATTR. Once #97109 lands, we should go and fix all uses of the old style to use this new style.

Copied from https://github.com/llvm/llvm-project/pull/97109#discussion_r1664851925, we should:

  1. Define new macro and update docs to say how to use it correctly. That can land without other blockers.
  2. Update clang-tidy checks to enforce the new standard, making sure they have full fixit implementation. That shouldn't land quite yet because then the libc linter bot will start complaining anew.
  3. Use clang-tidy to apply fixups to all libc code so it now conforms. No manual edits should be needed, but manual audit is needed to be sure it caught everything and the fixit edits were correct.
  4. Land 2. & 3. either in quick succession or as one commit, not sure how much it matters if everybody who would look at the libc linter bot knows what's being done.

git grep LIBC_NAMESPACE -- 'libc*.rst' shows other example snippets that are violating the new style requirement, so those should be updated too. We can fix that up when we land the changes to the clang-tidy check's implementation.

frobtech commented 3 days ago

I'm not sure I quite grok exactly what ImplementationInNamespaceCheck is enforcing now. I think it may need some more changes beyond using a different macro name (that's distinct from the CalleeNamespaceCheck one since one will be LIBC_NAMESPACE_DECL and the other LIBC_NAMESPACE).

I guess it is enforcing what we need, but not everywhere we need it. AIUI it requires that all declarations (whether defining or not) are in some namespace--but only in the main file, not in header files. We definitely need to enforce that the declarations in the "implementation" header files are all in the right namespace. I'm not sure how we sort out those headers from the ones meant to declare things outside the namespace, maybe we need to use pathname patterns?

If I'm reading its code right, it only checks that the namespace name is part of some macro expansion. We need it to ensure that it's actually LIBC_NAMESPACE_DECL and not a different macro. I think some more arcane corners of the MatchResult API probably have ways to work back to the name of the macro that got expanded.

It's also wholly lacking in fixit code. The most important case to get the fixit code right in is when there is a namespace decl but it's the wrong name, since that's how we can fix existing files en masse.

lntue commented 3 days ago

I'm not sure I quite grok exactly what ImplementationInNamespaceCheck is enforcing now. I think it may need some more changes beyond using a different macro name (that's distinct from the CalleeNamespaceCheck one since one will be LIBC_NAMESPACE_DECL and the other LIBC_NAMESPACE).

I guess it is enforcing what we need, but not everywhere we need it. AIUI it requires that all declarations (whether defining or not) are in some namespace--but only in the main file, not in header files. We definitely need to enforce that the declarations in the "implementation" header files are all in the right namespace. I'm not sure how we sort out those headers from the ones meant to declare things outside the namespace, maybe we need to use pathname patterns?

If I'm reading its code right, it only checks that the namespace name is part of some macro expansion. We need it to ensure that it's actually LIBC_NAMESPACE_DECL and not a different macro. I think some more arcane corners of the MatchResult API probably have ways to work back to the name of the macro that got expanded.

It's also wholly lacking in fixit code. The most important case to get the fixit code right in is when there is a namespace decl but it's the wrong name, since that's how we can fix existing files en masse.

If I remember it correctly, we wanted this check to make sure that the entrypoints are defined in LIBC_NAMESPACE?