[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D132513#3769267 <https://reviews.llvm.org/D132513#3769267>, @awarzynski wrote:

> Just a drive through review from me. Thanks for working on this!

Thanks for the review.



================
Comment at: llvm/docs/TestingGuide.rst:624-627
+  The number of the line where this substitution is used, with an
+  optional integer offset.  These expand only if they appear
+  immediately in ``RUN:``, ``DEFINE:``, and ``REDEFINE:`` directives.
+  Occurrences in substitutions defined elsewhere are never expanded.
----------------
awarzynski wrote:
> Why delete "This can be used in tests with multiple RUN lines, which reference test file's line numbers"?
It seems misleading to me: these substitutions can be used even if there's only one RUN line.  Beyond that issue, I'm not sure what the sentence says that the first sentence doesn't already say more clearly.

If I've misunderstood the sentence and lost an important point, please let me know.  However, if we keep it, it might need to be generalized to make sense for the new directives.


================
Comment at: llvm/docs/TestingGuide.rst:694-696
+    ; DEFINE: %{triple} =
+    ; DEFINE: %{cflags} =
+    ; DEFINE: %{fc-prefix} =
----------------
awarzynski wrote:
> What happens if these remain undefined? I couldn't find this documented or tested.
If, for example, `%{triple}` is left undefined, then the `REDEFINE:` directive for `%{triple}` below will report an error.  This error is documented under the `REDEFINE:` documentation entry below, and `llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-none.txt` tests that behavior.  Is that what you're looking for?


================
Comment at: llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define-redefine.txt:14
+# CHECK: Unresolved: 1
\ No newline at end of file

----------------
awarzynski wrote:
> FIXME
Ah, I thought I had fixed all these.  Will do in my next update.  Thanks.


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

https://reviews.llvm.org/D132513