asb added inline comments.
================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:50
+
+static void getExtensionVersion(StringRef In, std::string &Version) {
+ auto I = In.begin();
----------------
You should probably document the limitation that this doesn't currently parse
minor versions e.g. i2p0.
================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:144-146
+ // Currently LLVM does not support 'e'.
+ D.Diag(diag::err_drv_invalid_riscv_arch_name)
+ << MArch << "unsupported standard user-level extension 'e'";
----------------
This could be tightened up by also rejected rv64e as invalid.
================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:201-202
+ if (StdExtsItr == StdExtsEnd) {
+ size_t Pos;
+ if (hasExtension(StdExts, std::string(1, c), Pos)) {
+ D.Diag(diag::err_drv_invalid_riscv_ext_arch_name)
----------------
I'd suggest either just using StringRef::contains and getting rid of
hasExtension, or adding a doc comment to hasExtension to explain its semantics.
It might also be worth adding a comment to explain why you want to check the
extension is present in the StdExts string (e.g. We have reached the end of the
StdExts string. Either the current extension was given outside of the canonical
order (in which case issue an error), or else no canonical ordering is defined
meaning no error should be generated'.
================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:211
// Move to next char to prevent repeated letter.
++StdExtsItr;
----------------
Won't this now iterate StdExtsItr past StdExtsEnd if StdExtsItr == StdExtsEnd
but the hasExtension call is false?
================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:265-267
if (HasD && !HasF)
- D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+ D.Diag(diag::err_drv_invalid_riscv_arch_name) << MArch
+ << "d requires f extension to also be specified";
----------------
Add a TODO about other dependencies perhaps? e.g. ef and efd are invalid, and q
requires rv64.
https://reviews.llvm.org/D45284
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
