Details
Diff Detail
- Repository
- rLLD LLVM Linker
Event Timeline
| ELF/ScriptParser.cpp | ||
|---|---|---|
| 430 | I don't think returning a bfd name makes sense. You should simply return a boolean value indicating whether it is MIPS N32 ABI or not. But why MIPS always does too many things slightly differently? Do you really need to support both tradmips and ntradmips? | |
- Return a boolean value indicating whether the name is MIPS N32 ABI or not from the readBfdName .
I think one of the problem is that there are multiple really different architectures and ABIs under the same EM_MIPS name. In particular, maybe it was better to introduce two different names EM_MIPS and EM_MIPS_64 many years ago. In that case ntradmips (N32 ABI) could be describes like {ELF32LEKind, EM_MIPS_64}.
| ELF/ScriptParser.cpp | ||
|---|---|---|
| 388 | I would suggest doing something like: namespace {
struct BfdArchKind {
ELFKind BfdEKind;
uint16_t BfdEMachine;
bool IsMipsN32Abi;
};
};
static BfdArchKind parseBfdName(StringRef Name) {
...i.e. use a struct instead of std::tuple and a static helper instead of the member function. | |
| ELF/ScriptParser.cpp | ||
|---|---|---|
| 388 | It seems a bit more readable because it is sometimes unobvious what pair/tuple do contain. | |
| ELF/ScriptParser.cpp | ||
|---|---|---|
| 388 | I like an idea to use a helper structure, but unfortunately we cannot make parseBfdName a static helper because it calls private ScriptParser::next(). | |
I agree with that. Too many different things are categorized under the same name just because it is considered just "MIPS". That's unfortunate.
| ELF/ScriptParser.cpp | ||
|---|---|---|
| 428–435 | This is a bit shorter: std::tuple<ELFKind, uint16_t, bool> Tuple = readBfdName(); if (Config->EKind == ELFNoneKind) std::tie(Config->EKind, Config->EMachine, COnfig->MipsN32ABI) = Tuple; | |
As to defining a struct to store the parse result of parseBfdName, I don't think we need to define a new struct. That seems a bit too heavyweight to represent just a temporary value.
I would suggest doing something like:
namespace { struct BfdArchKind { ELFKind BfdEKind; uint16_t BfdEMachine; bool IsMipsN32Abi; }; }; static BfdArchKind parseBfdName(StringRef Name) { ...i.e. use a struct instead of std::tuple and a static helper instead of the member function.