AndrewRadev/sideways.vim

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.