[PATCH] D82923: introducing llvm-libtool-darwin

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

+1 to llvm-libtool-darwin. I think avoiding a name clash with GNU libtool in the future is highly desirable. If the two tools are unrelated, then using two different directories makes a lot of sense to me.

The pre-merge bot claims your help test is failing. You'll need to fix that.

In D82923#2126187 <https://reviews.llvm.org/D82923#2126187>, @smeenai wrote:

> Looking at libtool's options further, the only concern I have is with:
>
>   -      Treat all remaining arguments as names of files (or archives) and not as options.
>   
>
> In other words, if you include a single `-` by itself on your command line, all remaining arguments are parsed as input names. cl::opt supports similar functionality, but using `--` instead of `-`. I don't know if the TableGen parsing can support that either though, so I don't know if it would be possible for us to keep the exact same command line interface (with respect to this flag) anyway.


Thanks. To be clear, I fully support using the tablegen style if it will make things easier - @sameerarora101, you might want to experiment with this issue @smeenai has highlighted before this gets landed, to see which works better.

Also, if you haven't already, you might want to email that mailing thread/post a new email on llvm-dev highlighting that this review now exists so that any interested parties can be aware and chip in if they want.

I'd recommend starting a doc in the CommandGuide folder, as part of this change, referenced from the main page, and updating it as you go, rather than having to write it all at once. That way users will know how much this tool supports.



================
Comment at: llvm/tools/llvm-libtool-darwin/LLVMBuild.txt:1
+;===- ./tools/llvm-libtool-darwin/LVMBuild.txt --------------------------*- Conf -*--===;
+;
----------------
This line doesn't look quite right. It looks too long and might need fixing. Is it normal to include the path here also? That seems a little weird to me.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:13-24
+#include "llvm/Object/Archive.h"
+#include "llvm/Object/ArchiveWriter.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/MachO.h"
+#include "llvm/Object/MachOUniversal.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Option/Arg.h"
----------------
You probably can get rid of most of these.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:27
+using namespace llvm;
+using namespace llvm::object;
+
----------------
Again, this is redundant in this change.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:29-30
+
+// The name this program was invoked as.
+static StringRef ToolName;
+
----------------
This is not currently used by anything, so you can postpone adding it.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:32-40
+static cl::opt<std::string> OutputFile("output",
+                                       cl::desc("Specify output filename"),
+                                       cl::value_desc("filename"),
+                                       cl::OneOrMore);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
+                                 cl::aliasopt(OutputFile));
+
----------------
If you are going to add the input/output file options, you need test cases showing what is/isn't accepted in terms of number of arguments and option names.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:35
+                                       cl::value_desc("filename"),
+                                       cl::OneOrMore);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
----------------
Are you sure you want this as `OneOrMore`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82923