[llvm] [llvm-ml] Remove unsafe getCurrentSegmentOnly() call (PR #123355)

Eric Astor via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 17 14:39:09 PST 2025


ericastor wrote:

> The two commits intended to simplify MC internals. For the gas compatible AsmParser.cpp, we have comprehensive tests and passing the testsuite gives contributors very high confidence in the patches. I apologize if the simplification actually caused regression to MasmParser.cpp. That was not my intention.
> 
> That said, I should be honest. I am deeply concerned with the state of MasmParser.cpp. When MasmParser.cpp was forked from AsmParser.cpp, it copied a lot of code from AsmParser. A lot of pieces are unreachable or untested for llvm-ml.
> 
> I don't think it is reasonable to require contributors that refactor MC to understand llvm-ml internals and make perfect judgement on whether a refactoring will regress llvm-ml or not. You have to write tests to guard against regressions.

I agree. To be honest, I was surprised the test suite *didn't* already cover this case, so this was definitely a mistake on my part. I'm just interested which tests were broken with this refactor of MC, but I'm pleased to hear that they *were* broken and detected the issue.

In terms of reducing MasmParser.cpp... I agree that it could use a lot of cleanup. I would very much like to work on improving the test case coverage. Is there an easy way to get coverage reporting out of the LLVM test suite? I'm apparently not familiar with it, and since it's very much designed as more of an integration-test framework, I don't know any obvious tricks.

https://github.com/llvm/llvm-project/pull/123355


More information about the llvm-commits mailing list