[cfe-commits] r144269 - in /cfe/trunk: include/clang-c/ include/clang/Frontend/ lib/Frontend/ tools/c-index-test/ tools/libclang/

[Argyrios Kyrtzidis]

On Nov 10, 2011, at 10:49 AM, Ted Kremenek wrote:

> On Nov 10, 2011, at 10:32 AM, Argyrios Kyrtzidis wrote:
> 
>>> +void CXLoadedDiagnostic::decodeLocation(CXSourceLocation location,
>>> +                                        CXFile *file,
>>> +                                        unsigned int *line,
>>> +                                        unsigned int *column,
>>> +                                        unsigned int *offset) {
>>> +  
>>> +  
>>> +  // CXSourceLocation consists of the following fields:
>>> +  //
>>> +  //   void *ptr_data[2];
>>> +  //   unsigned int_data;
>>> +  //
>>> +  // The lowest bit of ptr_data[0] is always set to 1 to indicate this
>>> +  // is a persistent diagnostic.
>>> +  //
>>> +  // For now, do the unoptimized approach and store the data in a side
>>> +  // data structure.  We can optimize this case later.
>>> +  
>>> +  uintptr_t V = (uintptr_t) location.ptr_data[0];
>>> +  assert((V & 0x1) == 1);
>>> +  V &= ~(uintptr_t)1;
>>> +  
>>> +  const Location &Loc = *((Location*)V);
>>> +  
>>> +  if (file)
>>> +    *file = Loc.file;  
>>> +  if (line)
>>> +    *line = Loc.line;
>>> +  if (column)
>>> +    *column = Loc.column;
>>> +  if (offset)
>>> +    *offset = Loc.offset;
>>> +}
>> 
>> Is this modification to CXSourceLocation so that the diagnostic location becomes independent of a SourceManager ?
> 
> Yes.  The idea is that deserialized diagnostics don't have a source location.  They don't actually need one since all this locations are raw physical locations anyway.
> 
>> 
>> Why not introduce a new kind of location (like CXFileLocation or something) instead ?
> 
> I couldn't disagree with you more.  Requiring clients to use a different set of mostly duplicate APIs for source locations which are just different under the hood seems like extremely poor API design to me.  It makes the API verbose for no reason at all.  Why should the client be concerned at all about how the CXSourceLocation is implemented under the hood as long as it provides the same functionality?

I'm fine with abstracting under hood if the abstraction does not "leak"; if the plan is to make all the APIs that accept a CXSourceLocation work with this new kind that'd be great, otherwise it does not "provide the same functionality".

> 
> Keep in mind this would also require duplicating a bunch of the CXDiagnostics API to distinguish between diagnostics that have CXFileLocations and those that have CXSourceLocations.

No, you make the CXDiagnostics API use just CXFileLocations. Otherwise you get in a situation where to know whether you can or cannot do stuff with CXSourceLocations you need to know where they came from.

> 
>> If you remove the SourceManager you "disable" the functionality of CXSourceLocation anyway, e.g. while previously you could go through the expansion stack, get presumed location, etc., you can't do that with this new kind of location.
> 
> Sorry, but I think that statement is not true.  The prime use of CXSourceLocations is to query file, line, and column information.  That is still supported.

CXSourceLocation can do a lot more than that

We have

clang_getExpansionLocation(CXSourceLocation)
clang_getPresumedLocation(CXSourceLocation)
clang_getSpellingLocation(CXSourceLocation)

these are all different. There is also SourceManager::getFileLocation, which is currently not exposed by libclang but which could in the future, which is slightly different again (returns location of expansion or spelling depending on if the location is in a macro argument or not).

Also clang_getCursor(CXTranslationUnit, CXSourceLocation) does not work with the new kind.

> 
> We aren't disabling any functionality.  Yes, we are losing the macro expansion trace and the inclusion stack, but the locations are actual locations.  They are just the spelling locations.  In the absence of macro expansions, these are the same exact locations that a client would see in the non-deserialized diagnostics case.  One way to think of it is that the locations have been "pre-lowered" for consumption by a client that just wants to issue warnings at actual lines/columns in code.  We can still include all the macro expansion goop, but pre-lower those as parts of the diagnostic instead of encoding them into the CXSourceLocations.

So how do you see providing the macro expansion goop. Previously the client could use the clang_get*Location functions with a CXSourceLocation to get everything she is interested in.
If you are going to "pre-lower those as parts of the diagnostic", which means providing new API,  what is the practical advantage of giving a CXSourceLocation for a diagnostic.

> 
>> Seems better to be clear in the API.
> 
> I think duplicating most (all) of the same CXSourceLocation APIs just to expose a detail that clients should not be thinking about anyway adds no clarity to the API.

Currently the new kind only provides file, line, and column information; if there is no plan to support the other stuff you only need one new function for that.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111110/95d83b33/attachment.html>