[PATCH] [libclang] getTypeSize patch

[Argyrios Kyrtzidis]

On Feb 22, 2013, at 4:38 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Sat, Feb 23, 2013 at 1:22 AM, Sam Price <thesamprice at gmail.com> wrote:
>> Added a command to grab the the size, and the proper hooks to access it
>> within python bindings.
> 
> Hi Sam,
> 
> /**
> + * \brief Retrieve the size of the given type.
> + */
> +CINDEX_LINKAGE long long clang_getTypeSize(CXType T);
> 
> Is the size in bytes or bits?
> 
> Please also expand this comment to cover what we do for corner cases.
> I think that it is a good idea for this API to return exactly the same
> value that sizeof returns.  (But I don't think this is the case with
> current implementation -- consider function types, references,
> incomplete types, uninstantiated templates).  I understand that this
> is might be much more than you need to accomplish your task, but the
> feature should be implemented completely, or not done at all.
> 
> Argyrios, what is your preference on the semantics of this function?
> Should it try to return the same thing as sizeof?

Yes, that makes sense and is easy to understand what the function does.

Also how about enhancing it to also return alignment with something like this:
CXSizeOfTypeError clang_getSizeAndAlignOfType(CXType T, unsigned long long *size, unsigned long long *align)

The return value would be an enum indicating if the function was successful and if not what kind of error occurred.

-Argyrios

> 
> +    /*Print Size of type*/
> 
> It should be /* Print size of type. */ -- note spaces, capitalization,
> full stop.
> 
> +    {
> +      PrintTypeSize(T, "[typesize=%d]");
> +    }
> 
> (1) There should be a space in the string, " [typesize...
> (2) The format should be %lld
> (3) Just inline the PrintTypeSize.  There's only one call site, thus
> no need for this trivial function.
> 
> +long long clang_getTypeSize(CXType CT ) {
> 
> Extra space after CT.
> 
> +  CXTranslationUnit TU = GetTU(CT);
> +  ASTUnit *AU = clang::cxtu::getASTUnit(TU);
> +  const ASTContext &AC = AU->getASTContext();
> +  QualType T = GetQualType(CT);
> +  return AC.getTypeSize(T);
> +}
> 
> I did not run this, but it would probably crash on an incomplete type.
> Even if it doesn't, we should have a separate error code for that.
> 
> The actual tests are missing.  I think that current tests should have
> been broken by this, too, since you have changed the output format of
> c-index-test (this is the correct thing to do, but you need to update
> existing tests for the new information you print).
> 
> Python bindings also need tests -- see bindings/python/tests/ directory.
> 
> Dmitri
> 
> -- 
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/