-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[llvm][docs] Correct description of %t lit substitution #164397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
%t is currently documented as: temporary file name unique to the test Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is the path of a file.
|
@llvm/pr-subscribers-testing-tools Author: David Spickett (DavidSpickett) Changes%t is currently documented as: https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in #164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is the path of a file. Full diff: https://github.com/llvm/llvm-project/pull/164397.diff 1 Files Affected:
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 70daae46170cd..89adb003832ef 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -628,7 +628,7 @@ TestRunner.py:
%{fs-src-root} root component of file system paths pointing to the LLVM checkout
%{fs-tmp-root} root component of file system paths pointing to the test's temporary directory
%{fs-sep} file system path separator
- %t temporary file name unique to the test
+ %t the path of a temporary file unique to the test
%basename_t The last path component of %t but without the ``.tmp`` extension (deprecated, use ``%{t:stem}`` instead)
%% %
%/s %s but ``\`` is replaced by ``/``
|
mstorsjo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
c-rhodes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also LGTM cheers, although looks like the description needs updating to drop the mention it's a file (since it's just a path that becomes a file or directory depending on what you do with it):
Make it clear in the docs that this is the path of a file.
|
Since this is about wording, I'll leave this open for a day or two and see if anyone else chimes in. |
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as: temporary file name unique to the test https://llvm.org/docs/CommandGuide/lit.html#substitutions Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path. (which is hinted at by %basename_t, but why would you read that if you didn't need to use it) As seen in llvm#164396 this can create confusion when people use it as if it were just the file name. Make it clear in the docs that this is a unique path, which can be used to make files or folders.
%t is currently documented as:
temporary file name unique to the test
https://llvm.org/docs/CommandGuide/lit.html#substitutions
Which I take to mean if the path is a/b/c/tempfile, then %t would be tempfile. It is not, it's the whole path.
(which is hinted at by %basename_t, but why would you read that if you didn't need to use it)
As seen in #164396 this can create confusion when people use it as if it were just the file name.
Make it clear in the docs that this is a unique path, which can be used to make files or folders.