Changed Opts.EABIVersion type string to llvm::EABI enum class
ClosedPublic

Authored by yamaguchi on Jun 24 2017, 7:18 AM.

Details

Summary

Changed EABIVersion type from string to llvm::EABI.
It seems it was just a typo and this is intended implementation.

Diff Detail

Repository
rL LLVM
yamaguchi created this revision.Jun 24 2017, 7:18 AM
ruiu added a subscriber: ruiu.Jun 26 2017, 1:31 PM

Nice find!

Looks like the original intention of this code is to convert a string to an enum value and then initialize Opts.EABIVersion with the enum. So I wonder if we should change the type of Opts.EABIVersion (which is defined in clang/include/clang/Basic/TargetOptions.h) from std::string to llvm::EABI and keep the StringSwitch.

yamaguchi updated this revision to Diff 104976.Jun 30 2017, 7:48 PM

Update review and diff.

yamaguchi retitled this revision from Made OPT_meabi value handling simpler to Made Opts.EABIVersion a llvm::EABI enum class.Jun 30 2017, 7:50 PM
yamaguchi edited the summary of this revision. (Show Details)
yamaguchi added a reviewer: ruiu.
yamaguchi retitled this revision from Made Opts.EABIVersion a llvm::EABI enum class to Changed Opts.EABIVersion type string to llvm::EABI enum class .Jun 30 2017, 7:52 PM
ruiu added inline comments.Jun 30 2017, 7:56 PM
clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

How about assigning directly to Opts.EABIVersion? If you do, you can eliminate this temporary variable.

yamaguchi added inline comments.Jun 30 2017, 8:00 PM
clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

@ruiu
I thought about it, but I was not sure if we can set llvm::EABI::Unknown to Opts.EABIVersion.

ruiu added inline comments.Jun 30 2017, 8:02 PM
clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

In terms of your system, you can. And if you abort when it is Unknown, it doesn't matter whether it's unknown or not, no?

ruiu added inline comments.Jun 30 2017, 8:05 PM
clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

Sorry for the silly typo. I meant "in terms of the type system".

yamaguchi added inline comments.Jun 30 2017, 8:10 PM
clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

I'm not very happy about setting invalid value to Opts.EABIVersion, but I will change this if you strongly feel so.

ruiu accepted this revision.Jun 30 2017, 8:12 PM

LGTM

clang/lib/Frontend/CompilerInvocation.cpp
2550 ↗(On Diff #104976)

I'm fine with your code. Mine was just a suggestion.

This revision is now accepted and ready to land.Jun 30 2017, 8:12 PM
This revision was automatically updated to reflect the committed changes.