[PATCH] D73891: [RISCV] Support experimental/unratified extensions

[Simon Cook via Phabricator via cfe-commits]

simoncook marked 2 inline comments as done.
simoncook added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:50
 
+static bool isExperimentalExtension(StringRef Ext) {
+  // Currently 'b' is the only supported experimental extension
----------------
rogfer01 wrote:
> Suggestion: I think we can avoid these two functions (`isExperimentalExtension` and `getExperimentalExtensionVersion`) going out of sync if we have only one of them and make it return an `llvm::Optional` of the pair of versions.
> 
> Thent it can be used like this
> 
> ```lang=cpp
> if (auto ExperimentalExtension = isExperimentalExtension(Ext)) {
>    std::pair<StringRef, StringRef> SupportedVers = *ExperimentalVersion;
>    ...
> }
> ```
> 
> I'd also add a comment that the pair's `first` is the major version and `second` is the minor version (or alternative use a struct with two public fields `Major` and `Minor`)
That's a good idea, I'll update this to use this pattern.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:396
+      I += Minor.size() + 1 /*'p'*/;
+    if (*I == '_')
+      ++I;
----------------
rogfer01 wrote:
> There is no test for that case but I'm afraid we can't test it yet, can we?
We can test for e.g. `rv32i_m` which is now accepted (GCC also accepts this option we didn't before), but the version number code I don't think there is until we support versions on more than one extension.


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

https://reviews.llvm.org/D73891