[PATCH] D44727: [RISCV] Extend getTargetDefines for RISCVTargetInfo

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Do the macros you're defining here match gcc?



================
Comment at: lib/Basic/Targets/RISCV.cpp:68
+
+bool RISCVTargetInfo::hasFeature(StringRef Feature) const {
+  return llvm::StringSwitch<bool>(Feature)
----------------
asb wrote:
> It seems a number of other targets also return true for the archname, e.g. "riscv".
> 
> Is it the case that hasFeature should always support at least everything detected by handleTargetFeatures? If so, a comment to document this would be helpful.
> 
> I can see this method was introduced in rC149227 and is primarily motivated by module requirements.
> 
> @doug.gregor / @rsmith : could you please advise on the implementation and testing of this function? Which features should be hasFeature check for? I note that e.g. lib/Basic/Targets/ARM.cpp only supports a subset of the features in hasFeature compared to handleTargetFeatures.
I think your assessment is right... and also, it looks like we have poor test coverage for the set of features which are actually supported.  So probably some targets are missing relevant features.

Should be straightforward to test, I think; you just need to write a module.map with appropriate "requires" lines, and a test file which tries to include those modules.  See test/Modules/Inputs/module.map.


Repository:
  rC Clang

https://reviews.llvm.org/D44727