Hi Argyrios,
Thanks a ton for the review! I think the patch is much better now, for it.
Responses:
+ The convention in the libclang API is that if the function can return an
unambiguous invalid value, we do not + assert. If you document the behavior of
the function ("-1 is returned") you should allow the client to depend on it for
both debug and release.
+ I suggest removing the assertions in clang_Cursor_getNumTemplateArguments and
clang_Cursor_getTemplateArgumentType.
Done.
One question: ...getNumTemplateArguments and other functions depend on the
helper clang_Cursor_getTemplateArgument, which also has an unambiguous "bad"
return ("false"), so I've removed the asserts there, as well (or else
getNumTemplateArguments would still throw them). It seems like a shame, though,
to lose the debugging information that was captured in the assert message
("Invalid template argument index", for example, in the prior patch revision).
Is there a convention to preserve that information somehow, or is that not
standard Clang practice?
+ This indicates that we are missing another function to make using the others
convenient. We are missing a function to get the kind of the I'th template
argument, maybe have a function return if the I'th argument is a type or value ?
+ Also could you please add some tests for this new functionality, e.g. enhance
'test/Index/index-templates.cpp' ?
That's a good call - you're absolutely right. I've added
clang_Cursor_getTemplateArgumentKind and appropriate unit tests. Give them a
once-over when you get a chance.
While porting this change to cindex.py, I realized that there was a lot of
duplicated code for enumeration representation. I may have bitten off more than
I can chew, but I at least took a stab at unifying all that somewhat. It
definitely needs to be re-reviewed. :)
Again - thank you both for the reviews (updated patch coming shortly)!
Updated tests are still passing.
================
Comment at: bindings/python/tests/cindex/test_cursor.py:275
@@ +274,3 @@
+
+
+def test_get_template_argument_unsigned_value():
----------------
eliben wrote:
> One empty line here for consistency?
Done.
================
Comment at: include/clang-c/Index.h:3016
@@ +3015,3 @@
+ *
+ * If called with I = 1 or 2, 2^32 - 7 or true will be returned, respectively.
+ * For I == 0, an invalid value will be returned or an assert will be
triggered.
----------------
eliben wrote:
> I think an example with an actual unsigned would be more useful here :)
That's...a good point. Done.
http://reviews.llvm.org/D5621
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
