AsmAnalysis: Stronger use of fmt, refactor function call analysis #15941
Conversation
| std::optional<size_t> numReturns; | ||
| std::vector<std::optional<LiteralKind>> const* literalArguments = nullptr; | ||
|
|
||
| if (BuiltinFunction const* builtin = resolveBuiltinFunction(_funCall.functionName, m_dialect)) |
There was a problem hiding this comment.
this returns non-null IFF the _funCall.functionName variant holds a BuiltinName
clonker
force-pushed
the
asm_analysis_fmt
branch
from
ea0c63e to
e86a5f0
Compare
clonker
force-pushed
the
asm_analysis_fmt
branch
2 times, most recently
from
a77493e to
1caff77
Compare
libyul/AsmAnalysis.cpp
Outdated
| "An object name is only acceptable." | ||
| fmt::format( | ||
| "Data name \"{}\" cannot be used as an argument of eofcreate/returncontract. " | ||
| "An object name is only acceptable.", |
There was a problem hiding this comment.
Re-reading this, the word order is a bit off. I'd rather say "Only an object name is acceptable." Should we fix this here?
There was a problem hiding this comment.
Yeah, this wording is pretty awkward. Your version is much more comprehensible :) Let's fix it.
There was a problem hiding this comment.
I wonder if we could do something to make messages more consistent and comprehensible in general. They're often pretty bad. And they're the part of the compiler's UI that users see a lot, so it affects how users see us.
In PRs I usually have an urge to just rewrite all of them :) Though normally I try to limit myself to just to those where I can actually point to an actual issue (rather than just vaguely not liking how they sound).
Maybe at least for bigger features it would be good to go over them again when the feature is complete and we're adding docs? Especially putting them all side by side in a comment in such a PR could let us catch weirdness like this before it goes out to the world.
There was a problem hiding this comment.
Yes I think it is important to streamline them. As you said, they are user-facing and good error messages are super important. They make or break me wanting to continue using a software.
At some point it comes down to taste of course, I know the urge well :) Putting them into a comment and/or at least having a pass over them before releasing a feature into the wild sounds like a good idea. Besides the functionality itself, I would say it is one of the more important things to get right.
There was a problem hiding this comment.
Sounds good. Let's try that next time we do a feature that adds new messages.
clonker
force-pushed
the
asm_analysis_fmt
branch
from
1caff77 to
0e11b9f
Compare
cameel
force-pushed
the
asm_analysis_fmt
branch
from
0e11b9f to
29b6b09
Compare
clonker
force-pushed
the
asm_analysis_fmt
branch
from
29b6b09 to
6869617
Compare
clonker
force-pushed
the
asm_analysis_fmt
branch
from
6869617 to
814eb76
Compare
cameel
force-pushed
the
asm_analysis_fmt
branch
from
814eb76 to
b9cbf1e
Compare
This is so that the numerical id integration is more seamless.
string_views, ordinary string concatenation does not work anymore. I have replaced respective calls with format strings.