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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
