MaskRay accepted this revision. MaskRay added inline comments.
================ Comment at: clang/test/Driver/riscv-fixed-x-register.c:343 + +// Check that x18 is reserved on Android by default +// RUN: %clang --target=riscv64-linux-android -### %s 2> %t ---------------- asb wrote: > samitolvanen wrote: > > This seems redundant. Isn't the LLVM codegen test sufficient here? > I think testing that the `+reserve-x18` target feature is added by the > frontend probably has some value. > > I think it would better match the rest of this file to just do > `--check-prefix=CHECK-FIXED-X18` (I _think_ that works, but it is the end of > the day so I may be missing something obvious...). > > Please do try to remember to generate patches with full context (`-U999999` > or similar). I prefer shared CHECK prefixes `--check-prefix=CHECK-FIXED-X18` as well. We do that for many other tests. ================ Comment at: clang/test/Driver/riscv-fixed-x-register.c:343 + +// Check that x18 is reserved on Android by default +// RUN: %clang --target=riscv64-linux-android -### %s 2> %t ---------------- MaskRay wrote: > asb wrote: > > samitolvanen wrote: > > > This seems redundant. Isn't the LLVM codegen test sufficient here? > > I think testing that the `+reserve-x18` target feature is added by the > > frontend probably has some value. > > > > I think it would better match the rest of this file to just do > > `--check-prefix=CHECK-FIXED-X18` (I _think_ that works, but it is the end > > of the day so I may be missing something obvious...). > > > > Please do try to remember to generate patches with full context (`-U999999` > > or similar). > I prefer shared CHECK prefixes `--check-prefix=CHECK-FIXED-X18` as well. We > do that for many other tests. `/// Check that x18 is reserved on Android by default.` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145999/new/ https://reviews.llvm.org/D145999 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
