Skip to content

Conversation

PortalPete
Copy link
Contributor

The po alias now matches the behavior of the expression command when the it can apply a Fix-It to an expression.
Modifications

  • Add has m_fixed_expression to the CommandObjectDWIMPrint class a protected member that stores the post Fix-It expression, just like the CommandObjectExpression class.
  • Converted messages to present tense.
  • Add test cases that confirms a Fix-It for a C++ expression for both po and expressions

rdar://115317419

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this printed before or after the output? If it's printed after, past tense might make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only output.

Technically the print comes after the change, but it's all happening "now", in the present tense, from the developers perspective.

I can change it back to past tense, but I don't know that detail would be relevant to anyone other than someone working on LLDB itself. Even then, that person can just step through debugger and see when the change happened relative to the print statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed phrasing to past tense because this message appears after the expression's result.

error_stream << "  Evaluated this expression after applying Fix-It(s):\n";
error_stream << "    " << fixed_expression << "\n";

Copy link
Collaborator

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 understand what the second sentence is trying to convey.

Copy link
Contributor Author

@PortalPete PortalPete Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did my best to reword/convert this comment from CommandObjectExpression.cpp by:

  • Removing "we" to avoid 1st person narrative.
  • Avoiding future tense with "will".
  // We only tell you about the FixIt if we applied it.  The compiler errors
  // will suggest the FixIt if it parsed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! The original comment makes more sense: I think what it's trying to say is:

LLDB only displays fixits if the expression evaluator applied them. Any compiler diagnostics LLDB displays refer to the modified expression with the fixit applied.

Copy link
Contributor Author

@PortalPete PortalPete Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised comments.

// Only mention Fix-Its if the expression evaluator applied them.
// Compiler errors refer to the final expression after applying Fix-It(s).

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@felipepiovezan
Copy link
Contributor

The code is not something I am familiar with, but I did notice that the PR title and the commit title are quite different. Not sure which one is better, but it is generally nice to have them match (in particular it is nice to add the [lldb] tag to the commit title to make git log nice to browse)

Comment on lines 12 to 14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a multiline string, instead of using \ line continuations.

Suggested change
"Confirm that the `po` command (alias) applies a FixIt " \
"and prints it out to the console, " \
"just like the `expression` command."
"""
Confirm that the `po` command (alias) applies a FixIt
and prints it out to the console,
just like the `expression` command.
"""

Copy link
Contributor Author

@PortalPete PortalPete Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using a multiline string, but that adds hard returns and all the indentation spaces in the middle of the string, which gives you this string…

 Confirm that the `po` command (alias) applies a FixIt
        and prints it out to the console,
        just like the `expression` command.

Instead of this string …

 Confirm that the `po` command (alias) applies a FixIt and prints it out to the console, just like the `expression` command.

Plus, I noticed that running the single unit test only prints the string up until the first \n to the console, which means it's truncating everything after it.

The \ line continuations make it so that:

  • All the lines stay relatively short, just like a multiline string does.
  • The the test shows the whole string without truncating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with a """ string that's one line for doxygen.

"""Confirms `po` shows an expression after applying Fix-It(s)."""

@PortalPete
Copy link
Contributor Author

The code is not something I am familiar with, but I did notice that the PR title and the commit title are quite different. Not sure which one is better, but it is generally nice to have them match (in particular it is nice to add the [lldb] tag to the commit title to make git log nice to browse)

Thanks for the tip, @felipepiovezan .
All good now.

@PortalPete PortalPete force-pushed the dwim-print-fixit branch 3 times, most recently from 6ba636b to 6d8f611 Compare October 10, 2023 08:43
Modifying `po` alias to match outward FixIt behavior with `expression`.
- Fix `po` alias so that it prints out a message when applying a FixIt, just like the `expression` command.
- Add test cases for applying a FixIt with both `expression` command and `po` alias.
- Reword console messages for readability.
@adrian-prantl adrian-prantl merged commit 606f89a into llvm:main Oct 10, 2023
adrian-prantl added a commit that referenced this pull request Oct 10, 2023
…8452)"

This reverts commit 606f89a while investigating bot failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants