Fixes for obj properties by Krizai · Pull Request #1136 · uncrustify/uncrustify

Krizai · 2017-05-05T14:03:53Z

Based on the comments to #765:

Added missing properties
Fixed work of mod_sort_oc_properties with sp_after_comma

CDanU · 2017-05-05T15:21:25Z

Thanks for the fixes, the build currently fails because your changes have not been formatted according to this repos style format. Please use Uncrustify on the changed files with the forUncrustifySources.cfg config.

I will look into adding git hooks to prevent such issues in the future, most likely it will be a modification of https://github.com/githubbrowser/Pre-commit-hooks

Krizai · 2017-05-05T18:39:14Z

@CDanU Thank you. Now it's complaining about the output of Test_more_Options.sh script. I can't find information about the purpose of this script and the best way to make it happy. Can you help me with this ?

CDanU · 2017-05-05T18:47:56Z

This script tests if the command line options work.

Here is the script itself: Test_more_Options.sh

It operates on the files in scripts/Config, scripts/Output, scripts/Input and scripts/More_Options_to_Test/

You have changed/added some option descriptions and added new option so you need to edit

./scripts/More_Options_to_Test/help.txt
./scripts/More_Options_to_Test/show_config.txt
./scripts/Output/mini_d_uc.txt
./scripts/Output/mini_nd_uc.txt
./scripts/Output/mini_d_ucwd.txt
./scripts/Output/mini_nd_ucwd.txt

CDanU · 2017-05-05T19:34:33Z

Ohh, I forgot: You need to remove the version strings in the mentioned files

# Uncrustify-0.64-592-b7fc860-dirty

They are already removed via sed in the generated results files.

CDanU

Ok, after the scripts had their nitpicking now it's my turn, keep in mind that I never had to deal with obj-C.

CDanU · 2017-05-05T19:56:45Z

src/combine.cpp

+                  chunkGroup.push_back(next);
+                  nullability_chunks.push_back(chunkGroup);
+               }
+               else if (chunk_is_str(next, "null_unspecified", 16))


This could be combined with the previous else if statement.

CDanU · 2017-05-05T19:58:45Z

src/newlines.cpp

 // identifier       - the name of the enumeration that's being declared
 // enum-base(C++11) - colon (:), followed by a type-specifier-seq
+
+


This two newlines should be removed.

CDanU · 2017-05-05T20:01:52Z

src/tokenize_cleanup.cpp


+void cleanup_objc_property_with_parens(chunk_t *start);
+void mark_selectors_in_property(chunk_t *start);
+void mark_attributes_in_property(chunk_t *start);


Some documentation here would be nice.

CDanU · 2017-05-05T20:03:21Z

src/tokenize_cleanup.cpp

+{
+   chunk_t *next = chunk_get_next_ncnl(start);
+
+   set_chunk_parent(next, start->type);


This looks like it could need a nullptr check for next and start.

next should exist otherwise this method makes no sense, I'll put assert here

CDanU · 2017-05-05T20:05:59Z

src/tokenize_cleanup.cpp

+void mark_selectors_in_property(chunk_t *start)
+{
+   chunk_t *tmp = chunk_get_next_type(start, CT_PAREN_OPEN, start->level);
+


This looks like it could need a nullptr check for tmp and start.

paren open chunk should exist for calling this method. Will put assert

CDanU · 2017-05-05T20:09:02Z

src/tokenize_cleanup.cpp

+      if (tmp->type == CT_WORD &&
+          (memcmp(tmp->str.c_str(), "setter", 6) == 0 ||
+           memcmp(tmp->str.c_str(), "getter", 6) == 0)
+          && tmp->next->type == CT_ASSIGN)


I am not entirely sure, but this looks like it could need a nullptr check for next.
Does obj-C allow comments/newlines in between the setter and the assign ? Something like setter /* blub */ =?

Yes, will add.
Comments - never tried, but theoretically yes.

Ok, see if chunk_get_next_ncnl() is a better fit for such cases

Gets the next non-NEWLINE and non-comment chunk

Good one, thank you, will think about

CDanU · 2017-05-05T20:10:54Z

src/tokenize_cleanup.cpp

+           memcmp(tmp->str.c_str(), "getter", 6) == 0)
+          && tmp->next->type == CT_ASSIGN)
+      {
+         tmp = tmp->next->next;


One of both next can be nullptr or comment/newline

CDanU · 2017-05-05T20:11:52Z

src/tokenize_cleanup.cpp

+void mark_attributes_in_property(chunk_t *start)
+{
+   chunk_t *tmp = chunk_get_next_type(start, CT_PAREN_OPEN, start->level);
+


nullptr check for tmp and start

paren open chunk should exist for calling this method. Will put assert

CDanU · 2017-05-05T20:13:30Z

src/tokenize_cleanup.cpp

+   while (tmp->type != CT_PAREN_CLOSE)
+   {
+      if ((tmp->type == CT_COMMA || tmp->type == CT_PAREN_OPEN) &&
+          tmp->next->type == CT_WORD)


Also nullptr check and the in between comment/ newline problem for next

CDanU · 2017-05-05T20:13:53Z

src/tokenize_cleanup.cpp

+      if ((tmp->type == CT_COMMA || tmp->type == CT_PAREN_OPEN) &&
+          tmp->next->type == CT_WORD)
+      {
+         tmp->next->type = CT_OC_PROPERTY_ATTR;


nullptr check for next

Krizai · 2017-05-06T10:20:10Z

@CDanU Updated

CDanU

Ok, thanks again for the fixes, some new problems are there:

CDanU · 2017-05-06T15:36:47Z

src/tokenize_cleanup.cpp

+
+void cleanup_objc_property(chunk_t *start)
+{
+   chunk_t *open_paren = chunk_get_next_type(start, CT_PAREN_OPEN, start->level);


This will need a nullptr check for start.

CDanU · 2017-05-06T15:40:49Z

src/tokenize_cleanup.cpp

+{
+   chunk_t *tmp = open_paren;
+
+   assert(open_paren->type == CT_PAREN_OPEN);


You made sure that open_paren is not a nullptr in cleanup_objc_property(), but this function could be used somewhere else in the future where that is not going to be guaranteed anymore. A nullptr check for open_paren is needed here.

CDanU · 2017-05-06T15:41:05Z

src/tokenize_cleanup.cpp

+{
+   chunk_t *tmp = open_paren;
+
+   assert(open_paren->type == CT_PAREN_OPEN);


You made sure that open_paren is not a nullptr in cleanup_objc_property(), but this function could be used somewhere else in the future where that is not guaranteed anymore. A nullptr check for open_paren is needed here.

CDanU · 2017-05-06T16:35:31Z

Ok so far the code looks ok. Could you squash your commits into one commit?
I need to check if those asserts are not causing too much mess in there. This could potentially lead to problems if --no-backup is used.

@mihaipopescu could you also have a look on the changes?

Krizai · 2017-05-06T16:49:37Z

Asserts are only to detect wrong usage of the methods. They will never be executed with the current code.

mihaipopescu

Very good changes! Few remarks on the PR.

mihaipopescu · 2017-05-06T21:15:20Z

scripts/More_Options_to_Test/help.txt

      enable_processing_cmt and disable_processing_cmt).

-There are currently 581 options and minimal documentation.
+There are currently 582 options and minimal documentation.


Not related to this PR but generated files should never be under revision control.

mihaipopescu · 2017-05-06T21:17:30Z

src/combine.cpp

         while (next != nullptr && next->type != CT_PAREN_CLOSE)
         {
-            if (next->type == CT_WORD)
+            if (next->type == CT_OC_PROPERTY_ATTR)


Is guarded for OC only ? Can't really tell on my phone.

Yes. It's part of the method handle_oc_property_decl, which will be called only for CT_OC_PROPERTY

mihaipopescu · 2017-05-06T21:22:41Z

src/combine.cpp

            {
-               if (chunk_is_str(next, "atomic", 6))
+               if (chunk_is_str(next, "atomic", 6) ||
+                   chunk_is_str(next, "nonatomic", 9))


Not related to this PR but why do we have to pass strlen all the time instead of having a template function ?

mihaipopescu · 2017-05-06T21:26:22Z

src/tokenize_cleanup.cpp

+   while (tmp && tmp->type != CT_PAREN_CLOSE)
+   {
+      if (tmp->type == CT_WORD &&
+          (memcmp(tmp->str.c_str(), "setter", 6) == 0 ||


I think we have other string comparison functions.

true. Will change

mihaipopescu · 2017-05-06T21:28:10Z

src/tokenize_cleanup.cpp

+      if ((tmp->type == CT_COMMA || tmp->type == CT_PAREN_OPEN) &&
+          tmp->next && tmp->next->type == CT_WORD)
+      {
+         tmp->next->type = CT_OC_PROPERTY_ATTR;


You don't want to break the loop here ?

No, I want to do it for every attribute ( there can be several)

Fixed work of mod_sort_oc_properties with sp_after_comma

Krizai · 2017-05-06T22:17:40Z

Incorporated all the comments, except for the once unrelated to this PR.

CDanU · 2017-05-06T22:39:25Z

src/combine.cpp

+                        chunk_is_str(next, "nonnull", 7) ||
+                        chunk_is_str(next, "null_resettable", 15) ||
+                        chunk_is_str(next, "null_unspecified", 16)
+                        )


@gmaurel do we need a new rule in forUncrustifySources.cfg? I think that this paren should not be on a new line.

Yes, we need! look at #1139
I found some more:

grep -n " )$" * combine.cpp:2995: ) indent.cpp:1526: ) newlines.cpp:2168: )

Krizai force-pushed the fixes_for_objc_properties branch 2 times, most recently from 3acbc72 to b7fc860 Compare May 5, 2017 14:37

Krizai force-pushed the fixes_for_objc_properties branch from b7fc860 to 50e2bc9 Compare May 5, 2017 19:27

Krizai force-pushed the fixes_for_objc_properties branch 2 times, most recently from ca0fc00 to 996e218 Compare May 5, 2017 19:50

CDanU suggested changes May 5, 2017

View reviewed changes

Krizai force-pushed the fixes_for_objc_properties branch from d660ea4 to 13e0a80 Compare May 6, 2017 10:10

CDanU suggested changes May 6, 2017

View reviewed changes

Krizai force-pushed the fixes_for_objc_properties branch from 3899d70 to 99a8ba6 Compare May 6, 2017 16:47

mihaipopescu approved these changes May 6, 2017

View reviewed changes

Added missing properties

4891533

Fixed work of mod_sort_oc_properties with sp_after_comma

Krizai force-pushed the fixes_for_objc_properties branch from 99a8ba6 to 4891533 Compare May 6, 2017 22:16

CDanU approved these changes May 6, 2017

View reviewed changes

gmaurel merged commit 79ba971 into uncrustify:master May 8, 2017

		// identifier - the name of the enumeration that's being declared
		// enum-base(C++11) - colon (:), followed by a type-specifier-seq

Conversation

Krizai commented May 5, 2017

Uh oh!

CDanU commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Krizai commented May 5, 2017

Uh oh!

CDanU commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CDanU commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CDanU left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krizai commented May 6, 2017

Uh oh!

CDanU left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDanU commented May 6, 2017

Uh oh!

Krizai commented May 6, 2017

Uh oh!

mihaipopescu left a comment

CDanU commented May 5, 2017 •

edited

Loading

CDanU commented May 5, 2017 •

edited

Loading

CDanU commented May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU May 5, 2017 •

edited

Loading

CDanU left a comment •

edited

Loading

gmaurel May 8, 2017 •

edited

Loading