[PATCH] D116979: [llvm-ar] Add --thin for creating a thin archive

Tue Jan 11 08:54:11 PST 2022

MaskRay added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-ar.rst:204-205

- When creating or modifying an archive, this option specifies that the
- ``archive`` will be thin. By default, archives are not created as thin
- archives and when modifying a thin archive, it will be converted to a regular
- archive.
+ Deprecated alias for ``--thin``. In many ar implementations ``T`` has a
+ different meaning from X/Open System interface.

----------------
jhenderson wrote:
> "from" isn't the right word here, and because of that, I'm struggling to understand the meaning of this sentence. I'd suggest rewording.
> 
> I'm also somewhat opposed to deprecating the existing form. We have many users who are using thin archives, and will be using the T form to generate their archives. Deprecation implies that the option will be removed in a few releases time, but that would break all those users. Yes, it's a small change for them to make, but I don't really see the benefit. Most people probably don't care about the portability aspects of their build scripts.
> 
> Another related issue is that T is a modifier that is grouped with other operations and modifiers like 'r' and 'c', to form `Trc` or similar, whereas `--thin` is a long-form option. I suspect most llvm-ar users are more familiar with the short-form versions, and don't even know that llvm-ar has a long-form.
Reworded.

The deprecation semantic here is soft - no diagnostics. I think it will keep this way for a while. The Linux kernel uses it ([[ https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f5e181600954714a9e4c69f1f | kbuild: allow architectures to use thin archives instead of ld -r ]]) since 2016. 

elfutils has had the XSI -T since 2007. FreeBSD's and macOS's may be earlier. elfutils is used a lot on Linux, too, though its usage, compared with binutils, is fairly small. The thin/XSI conflict is unfortunate, and llvm-ar taking 'T' has some issues outside Linux (https://github.com/llvm/llvm-project/issues/25899#issuecomment-1009388242). If you find packages using T for thin archives, I'd still suggest that you try migrating away, if the available version (binutils>=2.38, llvm-ar>=14) is not a problem.

If you still think "Deprecated alias for --thin" is a problem, how about "Alias for --thin for compatibility"?

================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:122
   [S] - do not build a symbol table
-  [T] - create a thin archive
+  [T] - depreacted, use --thin instead
   [u] - update only [files] newer than archive contents
----------------
compnerd wrote:
> 
Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116979