[PATCH] Add warning capabilities in LLVM (backend part), Take 2

[Quentin Colombet]

Hi Alp,

On Dec 12, 2013, at 10:09 AM, Alp Toker <alp at nuanti.com> wrote:

> 
> On 11/12/2013 18:20, Quentin Colombet wrote:
>> Hi Renato,
>> 
>> On Dec 11, 2013, at 5:06 AM, Renato Golin <renato.golin at linaro.org> wrote:
>> 
>>> On 11 December 2013 12:52, Hal Finkel <hfinkel at anl.gov> wrote:
>>>>>> For what do you imagine that DiagnosticInfoOther will be used?
>>>>> I was thinking plugins.
>>> I think it should be easy for third-party implementations (not-yet or
>>> never-to-be merged code) to have their own non-clashing handlers,
>>> which ends up as deriving from the base class only on your tree and
>>> make use of it only by your code.
>> If everyone agrees we do not need the DiagnosticInfoOther, I am fine removing it.
> 
> Just for reference, these types of callback adapters tend to get called External* on the clang side.
> 
> e.g. ExternalSemaSource, ExternalIdentifierLookup, ExternalHeaderFileInfoSource.
> 
> Might be a good naming convention to follow.
If we want to keep it, that is a good idea indeed!

> 
> Personally I find them quite useful for trying things quickly, but I didn't have a chance to see what the impact is in your patch yet.
You can easily see the impact of this class on the patch by making a diff in phabricator between the last two revisions.
That said, the impact is small in my opinion.

Thanks for your feedbacks.

Cheers,
-Quentin

> 
> Alp.
> 
> 
> 
>> However, beyond the plugin case, I thought it would have been a good class to experiment things (like vectorizer information on why something has not been optimized using a DS_Note) or to report a richer message than InlineAsm without creating a specific subclass for each pattern.
>> 
>>> 
>>>> 1. enum DiagnosticKind should end with DK_FirstPluginKind
>>> This is ok, because plugins can use/reuse the same enum values, but it
>>> makes it hard for keeping trees in sync. Worst still, if you use two
>>> plugins that happen to use the same enum values, we'll only find out
>>> if they both run on the same binary and report an error.
>>> 
>>> Open source plugins can have their values added to the main tree,
>>> closed source ones cannot.
>> I think Hal’s suggestion is perfectly fine. The diagnostic ID in the plugins will be set at runtime using the getNextAvailablePluginDiagnosticKind(). Therefore, I do not see any problem here, except maybe thread safety when updating the next available diagnostic kind.
>> 
>>> 
>>>> 2. Add a global function: int getNextAvailablePluginDiagnosticKind() which returns an incremented static variable (which starts with DK_FirstPluginKind).
>>> I think we can start simple, and add a DK_PluginKind and make all
>>> plugins differentiate themselves in the error message,
>> The only difference I see here with the current DK_Other, is the name in the enum :).
>> 
>>> or even having
>>> a customized payload for the C-API which passes a PDK_Kind (Plugin
>>> Diagnostic Kind). But I think this needs more discussion.
>> For now, I think Hal’s suggestion is a good start. That said, I was not looking in addressing the problem adding new diagnostics at runtime. Thus, we may need another thread to discuss that.
>> I do not think that was block the current proposal.
>> 
>> What do you think?
>> 
>> Cheers,
>> Quentin
>>> cheers,
>>> --renato
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> -- 
> http://www.nuanti.com
> the browser experts
>