c++ template parameters are not handled properly
scottbreyfogle opened this issue · 3 comments
I made a change to fix this. Before I submitted a PR, I realized that it would cause problems when '>' and '<' were used as comparators inside of an argument.
Is there any acceptable way to distinguish between comparators and angle brackets? I think a 100% correct solution would require a full parse of the c++ code. I'm just wondering if there is a approximation that is better than just ignoring templates altogether.
I can't really think of anything good, to be honest. One thing I can say is that, in C++ in particular, it should be much more common to use angle brackets for type params than it would be for comparison operators, at least as arguments to functions.
One thing that would help a little bit is removing the definition that starts with <
. Right now, this is how your definitions work:
function(1 < 2, std::unordered_map<k, v>(), arg);
// Move the second argument to the left, and:
function(1 < std::unordered_map<k, v>(), 2, arg);
But, if you remove the last definition, this case seems to work fine. The <
is considered a bracket, but, because there's no matching closing one, the plugin still manages to work with it. The last definition considers the <
to be the beginning of a bracket set. Removing it would mean you couldn't use sideways with template arguments, but that might be fine? I guess it depends on how often you edit those.
Alternatively, the last definition could look like this:
\ {
\ 'start': '<\ze\S',
\ 'end': '>',
\ 'delimiter': ',\_s*',
\ 'brackets': ['([{<''"', ')]}>''"'],
\ },
The difference is that the start pattern is <\ze\S
, which ensures that the angle bracket is immediately followed by a non-whitespace character. This would mean that a < b
is not detected as a list start, while a<b, c>
works as a list with b
and c
.
Either way, though, this is impossible to handle:
function(std::unordered_map<k, v>(), 1 < 2, 3 > 4);
I mean, it kind of works for some things, because it just considers 1 < 2, 3 > 4
as one item. But yeah, it doesn't really work. And I can't think of any heuristic here. The brackets are characters that I check one by one, in pairs, so I can't really modify that pattern (to add the whitespace check), other than making a special case for angled brackets, somehow. It's doable, I guess, but it'd be a hack that I'd rather avoid, and it really feels like an edge case.
What do you think? If we change the <
pattern to <\ze\S
, relying on convention, that might be acceptable? Even with the edge case of two angled brackets in two separate arguments.
I support introducing the <\ze\S
as a sane default for C++. I am finding this plugin extremely useful and was surprised that it doesn't handle template parameters out of the box.
The use case with comparison operators as actual arguments is, IMHO, a really infrequent occurence.
If anything, it's for the better that it encourages you to write the argument above the call - with a proper name to it.
The only problem that I see is with something like this:
some_specialization<some_constant > 0>
Thus, if there's an actual arithmetic done in the template argument list, we're screwed.
I can see it being fixed by making the end pattern ensure that the >
is preceded by a non-whitespace character, too.
@geneotech @scottbreyfogle I've pushed Scott's definitions to master, with the exception of "<", which I've defined as \S\zs<\ze\S
. Basically, the <
character needs to be surrounded by non-whitespace on both sides, which I hope will always be the case for pretty much all C++ styles.
Thus, if there's an actual arithmetic done in the template argument list, we're screwed.
I can see it being fixed by making the end pattern ensure that the > is preceded by a non-whitespace character, too.
Unfortunately, I can't do this at the moment. The plugin goes over the text piece by piece, counting brackets, and it checks if the remainder of the line starts with the end delimiter. I can't say "does it match the end delimiter with a previously-consumed character being non-whitespace". I could rewrite this part to not use a string, but juggle buffer positions instead and call search()
-es. But I feel that it's enough of an edge case that I don't want to put that much work into it.
I recently added Rust support for template arguments, and it definitely feels nice. It's a lot easier there, though -- templates there don't allow arbitrary expressions, only types. And function invocations aren't called with foo<bar, baz>()
, but with foo::<bar, baz>()
, so that gives me some way to tell what it is.
Anyway, I'm going to close the issue for now, because I feel this is a good enough compromise. If and when there are bugs in this, or it turns out there are common cases that break things, feel free to reopen, or open a new issue.