-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[TVMScript] Text underlining in DocPrinter based on Doc's source_paths #12344
Conversation
num_spaces -= 1; | ||
printed_extra_caret = false; | ||
} | ||
*out += std::string(num_spaces, ' '); |
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.
Should it skip printing carets under the indentation spaces for multi-line underline?
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.
I'm not sure, I could see arguments either way.
for i in T.serial(10):
^^^^^^^^^^^^^^^^^^^^^^
a[i] = 5
^^^^^^^^^^^^
for i in T.serial(10):
^^^^^^^^^^^^^^^^^^^^^^
a[i] = 5
^^^^^^^^
I'd say that the first option can provide a better sense of "continuity". make it clear that this is one chunk of text being highlighted, as opposed to two different chunks. Also it seems to simplify the implementation :)
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.
This makes sense. What I was thinking about is the deeply nested code, like
for i in T.serial(10):
^^^^^^^^^^^^^^^^^^^^^^
a[i] = 5
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
b[i] = 6
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But it does add more complexity to the implementation. We can keep it as is for now and make improvement in the future if needed.
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.
Good point about the nested code. But let's give the simpler implementation a shot and then add more complexity if people hate this behavior.
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
Let's fix the lint and get it in :-) |
Thanks for the PR! It's merged :-) |
apache#12344) This adds an ability to print a "diagnostic marker" based on a given ObjectPath. For example, say we are printing a fragment of TIR like ``` for i in T.serial(10): a[i] = 5 ``` and we would like bring the user's attention to the bound of the loop: ``` for i in T.serial(10): ^^ a[i] = 5 ``` In this case we would give the doc printer an object path that represents this loop bound, i.e. something like `path_to_underline=ObjectPath.root().attr("extent")` Tracking issue: apache#11912
apache#12344) This adds an ability to print a "diagnostic marker" based on a given ObjectPath. For example, say we are printing a fragment of TIR like ``` for i in T.serial(10): a[i] = 5 ``` and we would like bring the user's attention to the bound of the loop: ``` for i in T.serial(10): ^^ a[i] = 5 ``` In this case we would give the doc printer an object path that represents this loop bound, i.e. something like `path_to_underline=ObjectPath.root().attr("extent")` Tracking issue: apache#11912
This adds an ability to print a "diagnostic marker" based on a given ObjectPath. For example, say we are printing a fragment of TIR like
and we would like bring the user's attention to the bound of the loop:
In this case we would give the doc printer an object path that represents this loop bound, i.e. something like
path_to_underline=ObjectPath.root().attr("extent")
Tracking issue: #11912
cc @yelite @junrushao1994