Skip to content

Internal validator error messages don't need /Zi anymore.#3606

Merged
adam-yang merged 11 commits into
microsoft:masterfrom
adam-yang:line_number_wo_zi
Mar 21, 2021
Merged

Internal validator error messages don't need /Zi anymore.#3606
adam-yang merged 11 commits into
microsoft:masterfrom
adam-yang:line_number_wo_zi

Conversation

@adam-yang
Copy link
Copy Markdown
Contributor

@adam-yang adam-yang commented Mar 18, 2021

No description provided.

@adam-yang adam-yang requested review from pow2clk and tex3d March 18, 2021 21:50
Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

I really like the look of these changes. I'm impressed it didn't take more invasive changes than this.

You refer to tests that have errors referencing Zi. Do you mean to remove the suggestion to add Zi from those error messages? That would be great. Sadly, we sometimes emit them even when they don't make any difference in the ability to print line numbers 😯

@AppVeyorBot
Copy link
Copy Markdown

@adam-yang
Copy link
Copy Markdown
Contributor Author

adam-yang commented Mar 18, 2021

I really like the look of these changes. I'm impressed it didn't take more invasive changes than this.

You refer to tests that have errors referencing Zi. Do you mean to remove the suggestion to add Zi from those error messages? That would be great. Sadly, we sometimes emit them even when they don't make any difference in the ability to print line numbers 😯

Yeah I want to test that we actually print out the line numbers correctly even withou Zi. Which cases does Zi not help? Maybe this change would fix it.

@pow2clk
Copy link
Copy Markdown
Collaborator

pow2clk commented Mar 18, 2021

I fixed the case I remember clearly, but I have a vague recollection that it's sometimes the case.

@pow2clk
Copy link
Copy Markdown
Collaborator

pow2clk commented Mar 18, 2021

The case I remember was trying to print the line number of an input variable that just didn't have one debug or no. It's possible that crops up in other areas. Irritating as an error message that gives no location information is, I think it's more irritating to be prompted to add a Zi flag when 1) you have or 2) you try it and it doesn't help 😞

@adam-yang adam-yang changed the title [WIP] Internal validator error messages don't need /Zi anymore. Internal validator error messages don't need /Zi anymore. Mar 19, 2021
@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

I think it looks good!

std::unique_ptr<llvm::MemoryBuffer> pMemBuf;
LLVMContext ctx;
if (pDebugModule) {
pMemBuf = llvm::MemoryBuffer::getMemBuffer(StringRef((char *)pDebugModule->Ptr, pDebugModule->Size), "", false);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As the clang error implies, this will need to be cast to (const char*) with that change, it builds with clang

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, and that reminds me about other issues which make it necessary to refactor this a bit more in any case.

@tex3d tex3d self-requested a review March 20, 2021 09:50
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

I think this is good now if it passes CI.

@AppVeyorBot
Copy link
Copy Markdown

@adam-yang adam-yang merged commit 86104f4 into microsoft:master Mar 21, 2021
tex3d pushed a commit to tex3d/DirectXShaderCompiler that referenced this pull request Mar 23, 2021
tex3d added a commit that referenced this pull request Mar 23, 2021
)

(cherry picked from commit 86104f4)

Co-authored-by: Adam Yang <31109344+adam-yang@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants