[cfe-commits] r165091 - /cfe/trunk/lib/AST/MicrosoftMangle.cpp

[Aaron Ballman]

On Wed, Oct 3, 2012 at 8:50 AM, Nico Weber <thakis at chromium.org> wrote:
> On Wed, Oct 3, 2012 at 9:40 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Wed, Oct 3, 2012 at 8:16 AM, Nico Weber <thakis at chromium.org> wrote:
>>> On Wed, Oct 3, 2012 at 9:05 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>> On Wed, Oct 3, 2012 at 2:12 AM, Nico Weber <nicolasweber at gmx.de> wrote:
>>>>> Author: nico
>>>>> Date: Wed Oct  3 01:12:27 2012
>>>>> New Revision: 165091
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=165091&view=rev
>>>>> Log:
>>>>> Replace a default: with an explicit list of cases. No functionality change.
>>>>>
>>>>> Modified:
>>>>>     cfe/trunk/lib/AST/MicrosoftMangle.cpp
>>>>>
>>>>> Modified: cfe/trunk/lib/AST/MicrosoftMangle.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/MicrosoftMangle.cpp?rev=165091&r1=165090&r2=165091&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/AST/MicrosoftMangle.cpp (original)
>>>>> +++ cfe/trunk/lib/AST/MicrosoftMangle.cpp Wed Oct  3 01:12:27 2012
>>>>> @@ -810,7 +810,12 @@
>>>>>          break;
>>>>>        }
>>>>>        /* fallthrough */
>>>>> -    } default: {
>>>>> +    }
>>>>> +    case TemplateArgument::Template:
>>>>> +    case TemplateArgument::TemplateExpansion:
>>>>> +    case TemplateArgument::Declaration:
>>>>> +    case TemplateArgument::NullPtr:
>>>>> +    case TemplateArgument::Pack: {
>>>>>        // Issue a diagnostic.
>>>>>        DiagnosticsEngine &Diags = Context.getDiags();
>>>>>        unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
>>>>
>>>> Now if another value is added to the list, it will silently fail; is
>>>> this acceptable?  I would feel more comfortable if there was still a
>>>> default case that would be marked as unreachable.
>>>
>>> It won't silently fail, clang will warn that a enum case isn't handled.
>>
>> Clang will, but not all compilers will.  MSVC doesn't warn on that
>> situation, for instance.  Marking default with llvm_unreachable solves
>> that by ensuring there's no warning, and by complaining loudly if the
>> default is ever hit (so you can't ignore it).
>
> Right, that way it's only caught at runtime, not compile time. Isn't that worse?
>
> From what I can tell, exhaustive switches without defaults are very
> common in clang's codebase.

I stand corrected -- we turn this feature *on* in clang with -w14062
as an additional option.  So this will fire by default in MSVC.  I'd
say your change looks great then!

~Aaron