Quirky diagnostic/fixit when using insertions

I came across the following diagnostic output when I examined the output of test/FixIt/fixit-cxx0x.cpp:

/tmp/webcompile/_934_0.cc:8:3: error: switch condition type 'foo' requires explicit conversion to 'int'
  switch(foo())
  ^
         stati)_cast<int>(

Which shows two problems:

  1. Multiple carets points don’t seem to be shown, only the first one (in this case, the location of the diagnostic, not the location of either of the suggested FixIts)
  2. If two FixIts overlap, in this case causing the second (“)”) to be embedded inside the first (replacing the “c” in “static”)

Are these known issues? Has there been any thought/discussion on how they should be fixed? (I imagine fixing (1) shouldn’t be terribly hard, but I haven’t looked. Though (2) will presumably require slightly more work to shift the fixits across and make it clear which piece is to be inserted where)

  • David

Hi,
Yes I also noticed this problem last week.
I filled a bugzilla:
http://llvm.org/bugs/show_bug.cgi?id=10696

/tmp/webcompile/_934_0.cc:8:3: error: switch condition type ‘foo’ requires
explicit conversion to ‘int’
switch(foo())
^
stati)_cast(

Which shows two problems:

  1. Multiple carets points don’t seem to be shown, only the first one (in
    this case, the location of the diagnostic, not the location of either of the
    suggested FixIts)
  2. If two FixIts overlap, in this case causing the second (“)”) to be
    embedded inside the first (replacing the “c” in “static”)

Are these known issues? Has there been any thought/discussion on how they
should be fixed? (I imagine fixing (1) shouldn’t be terribly hard, but I
haven’t looked. Though (2) will presumably require slightly more work to
shift the fixits across and make it clear which piece is to be inserted
where)

  • David

Hi,
Yes I also noticed this problem last week.
I filled a bugzilla:
http://llvm.org/bugs/show_bug.cgi?id=10696

Thanks - that’s (2) at least. Following on from the bug discussion I think it’d be a pity to drop the fixit entirely - though better than having it broken like that, sure.

Could we toss around some alternative syntax options & see what people think?

A var = { b};
          ^
          s)atic_cast<unsigned int>(
One option would be to just suggest the whole replacement (not by changing the FixIt emitted - that should remain the same so it's fully descriptive for other tools. But just to change the way its rendered):
A var = { b};
          ^
          static_cast<unsigned int>(b)
or somehow attach the start & end separately:
A var = { b};
          ^^-------------------------\
          static_cast<unsigned int>( )
Though that doesn't really scale up to more than one overlap. (which might be sufficient to cover more/most cases like this)
A simple separated list (could benefit from color highlighting, but we wouldn't want to depend on that)
A var = { b};
          ^^
          static_cast<unsigned int>( )
Is there any good separator we could use in this list that wouldn't get confused with the actual replacement text?

(could we even render fixits completely differently in the presence of terminal colors? 
A var = { **static_cast<unsigned int>(**b**)**};
Though I suppose this doesn't account for replacements (where we'd want to show the old & the new text) in which case we could pad the original text:

A var = { foo   (b)};
          foobar

)




FYI, I’ll try to comment later on some of the details of these syntaxes, but this is something I’ve wanted to look into as well.

Currently I’m working on refactorings to the text diagnostic printer that should make experimenting with these alternate syntaxes much much more reasonable.