This revision was automatically updated to reflect the committed changes.
Closed by commit rL322540: [libclang] Add PrintingPolicy for pretty printing
declarations (authored by jbcoe, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
nik added a comment.
> I can merge this for you.
> Please add me as reviewer in any follow-up patches and we can turn them
> around more quickly.
That would be nice, thanks! I don't have any follow-up patches right now.
Repository:
rC Clang
https://reviews.llvm.org/D39903
jbcoe added a comment.
I can merge this for you.
Please add me as reviewer in any follow-up patches and we can turn them around
more quickly.
Repository:
rC Clang
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
nik added a comment.
Can you submit this for me? I don't have the permissions.
Repository:
rC Clang
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
nik updated this revision to Diff 129805.
nik added a comment.
Addressed inline comment.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
tools/c-index-test/c-index-test.c
tools/libclang/CIndex.cpp
jbcoe added inline comments.
Comment at: tools/c-index-test/c-index-test.c:93
+enum CXPrintingPolicyProperty property;
+ } mappings[] = {
+ {"CINDEXTEST_PRINTINGPOLICY_INDENTATION", CXPrintingPolicy_Indentation},
I'm not sure that joining the struct
nik updated this revision to Diff 129635.
nik added a comment.
What about this? :)
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
tools/c-index-test/c-index-test.c
tools/libclang/CIndex.cpp
jbcoe requested changes to this revision.
jbcoe added inline comments.
This revision now requires changes to proceed.
Comment at: unittests/libclang/LibclangTest.cpp:596
+TEST_F(LibclangPrintingPolicyTest, GetProperty) {
+ EXPECT_EQ(2U, clang_PrintingPolicy_getProperty(Policy,
nik updated this revision to Diff 129600.
nik marked an inline comment as done.
nik added a comment.
Added assert() for getter/setter.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
nik marked an inline comment as done.
nik added inline comments.
Comment at: tools/libclang/CIndex.cpp:4782
+
+ return 0;
+}
jbcoe wrote:
> Might be worth asserting here.
Good idea. I've done the same for the setter.
Comment at:
jbcoe added a comment.
Looking good, only a few nits.
Comment at: tools/libclang/CIndex.cpp:4782
+
+ return 0;
+}
Might be worth asserting here.
Comment at: unittests/libclang/LibclangTest.cpp:596
+TEST_F(LibclangPrintingPolicyTest,
nik updated this revision to Diff 129591.
nik added a comment.
Addressed comments.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
tools/c-index-test/c-index-test.c
tools/libclang/CIndex.cpp
nik added a comment.
> It might be worth adding some very simple get/set tests to ensure that
> properties are set as intended.
clang_PrintingPolicy_setProperty is already called in c-index-test.c and
covered with test/Index/print-display-names.cpp. Do you have another kind of
test in mind?
jbcoe added inline comments.
Comment at: tools/libclang/libclang.exports:365
+clang_PrintingPolicy_get
+clang_PrintingPolicy_set
+clang_PrintingPolicy_dispose
clang_PrintingPolicy_setProperty and clang_PrintingPolicy_getProperty might be
better names (I know
jbcoe added a comment.
It might be worth adding some very simple get/set tests to ensure that
properties are set as intended.
Comment at: tools/libclang/CIndex.cpp:4720
+
+#define HANDLE_CASE(P, PROPERTY_NAME)
\
+ case
nik updated this revision to Diff 129427.
nik added a comment.
Used macros as in a previous version to make it less verbose and error prone.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
nik updated this revision to Diff 129426.
nik added a comment.
> Could one use an enum to get/set different properties of the policy?
>
> I've seen other C-API's (for Linear and Quadratic programming) follow a
> similar approach quite extensibly.
>
> It would significantly reduce the size of
jbcoe added inline comments.
Comment at: include/clang-c/Index.h:4118
+ */
+CINDEX_LINKAGE unsigned clang_PrintingPolicy_getIndentation(CXPrintingPolicy);
+
Could one use an enum to get/set different properties of the policy?
```
nik updated this revision to Diff 129258.
nik added a comment.
Rebased only, please review.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
tools/c-index-test/c-index-test.c
tools/libclang/CIndex.cpp
nik added a comment.
Ping...
Repository:
rC Clang
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
nik added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
nik added a comment.
In https://reviews.llvm.org/D39903#944182, @cameron314 wrote:
> Locally we've done something similar (adding a
> `clang_getCursorPrettyPrintedDeclaration` function, though without exposing
> the `PrintingPolicy`) and overhauled `DeclPrinter` to produce proper pretty
>
cameron314 added a comment.
Locally we've done something similar (adding a
`clang_getCursorPrettyPrintedDeclaration` function, though without exposing the
`PrintingPolicy`) and overhauled `DeclPrinter` to produce proper pretty names.
This is a hack that works well for us, but can never be
nik updated this revision to Diff 125129.
nik added a comment.
Rebased only.
Repository:
rC Clang
https://reviews.llvm.org/D39903
Files:
include/clang-c/Index.h
test/Index/print-display-names.cpp
tools/c-index-test/c-index-test.c
tools/libclang/CIndex.cpp
nik added reviewers: ilya-biryukov, cameron314.
nik added a comment.
Anyone?
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
nik added a comment.
Ping III - is there anything I can do to get this reviewed faster? 3 weeks
passed.
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
nik added a comment.
Ping, please review or add more appropriate reviewers.
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
nik added a comment.
Ping, please review :)
https://reviews.llvm.org/D39903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
28 matches
Mail list logo
