Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NULL check #1656

Merged
merged 2 commits into from
Jan 13, 2018
Merged

Fix NULL check #1656

merged 2 commits into from
Jan 13, 2018

Conversation

siiky
Copy link
Contributor

@siiky siiky commented Jan 10, 2018

Fix #1655

@siiky siiky changed the title Fix #1655 Fix NULL check Jan 10, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.414% when pulling 089e54d on SiIky:master into 796684b on universal-ctags:master.

@masatake masatake requested a review from pragmaware January 11, 2018 02:19
@masatake
Copy link
Member

[jet@localhost]~/var/ctags% cat /tmp/foo.cc 
cat /tmp/foo.cc 
namespace S {
  struct T {
    enum E {
      TOKYO, KYOTO,
    } elt;
  };
}

struct S::T s = { .elt = S::T::E::TOKYO };

[jet@localhost]~/var/ctags% diff -uN WITHOUT-PATCH.tags WITH-PATCH.tags 
diff -uN WITHOUT-PATCH.tags WITH-PATCH.tags 
--- WITHOUT-PATCH.tags	2018-01-11 11:54:21.299130364 +0900
+++ WITH-PATCH.tags	2018-01-11 11:54:33.683074701 +0900
@@ -11,9 +11,9 @@
 E	/tmp/foo.cc	/^    enum E {$/;"	g	struct:S::T	file:
 S::T::E	/tmp/foo.cc	/^    enum E {$/;"	g	struct:S::T	file:
 TOKYO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
-S::TOKYO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
+S::T::TOKYO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
 KYOTO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
-S::KYOTO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
+S::T::KYOTO	/tmp/foo.cc	/^      TOKYO, KYOTO,$/;"	e	enum:S::T::E	file:
 elt	/tmp/foo.cc	/^    } elt;$/;"	m	struct:S::T	typeref:enum:S::T::E	file:
 S::T::elt	/tmp/foo.cc	/^    } elt;$/;"	m	struct:S::T	typeref:enum:S::T::E	file:
 s	/tmp/foo.cc	/^struct S::T s = { .elt = S::T::E::TOKYO };$/;"	v	typeref:struct:S::T

The result is obviously improved.
S::T::E::TOKYO is not included as a tag but it seems that it is intended.

	if(eScopeType == CXXScopeTypeEnum)
	{
		// If the scope kind is enumeration then we need to remove the
		// last scope part. This is what old ctags did.
		if(cxxScopeGetSize() < 2)
			return -1; // toplevel enum

Actually, with your patch, u-ctags generates the same tags file as e-ctags generates.

I will a pull request to your branch. So could you push it here?

(
BTW, I think you should not use master branch on your side.
As far as I know, the master branch should be used for synchronizing upstream.
If you modify your master branch with a hacking activity, you cannot do another hack.
)

masatake added a commit to masatake/ctags that referenced this pull request Jan 11, 2018
@masatake
Copy link
Member

@masatake
Copy link
Member

(Thank you for pushing my change.)

@siiky
Copy link
Contributor Author

siiky commented Jan 11, 2018

Merged

I think you should not use master branch on your side

Sorry about that, next time I'll do it right. (This was my first PR)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.495% when pulling 63b24e2 on SiIky:master into 796684b on universal-ctags:master.

@masatake
Copy link
Member

Let's wait for the response from the maintainer of C/C++ parsers.

(This was my first PR)

I'm very proud of u-ctags project attracting a new hacker.
Though the required knowledge for developing u-ctags is only C and standard C library, I think developing u-ctags is very ... attractive. I cannot find a good word representing how I feel.

I hope that you enjoy hacking u-ctags more. So I would like to add more comments here.
I would like you to add a prefix to the subject of commit log like:

C++: fix NULL check

Here C++ is the prefix. If a patch is about a parser, use the name of parser as prefix.
Use main for the change for the common part of code. Please, see the other commit logs about more examples.

Knowing how to write a test case is important in this project. Please look at my patch which I submitted to your branch. See also http://docs.ctags.io/en/latest/units.html .

Thank you.

siiky and others added 2 commits January 11, 2018 23:45
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.495% when pulling a877beb on SiIky:master into 796684b on universal-ctags:master.

@pragmaware
Copy link
Contributor

Yep, looks good!
It's interesting that this bug didn't pop out earlier... it's there since the beginning of the parser.

@siiky
Copy link
Contributor Author

siiky commented Jan 12, 2018

(Didn't have time to reply yesterday)

I'm very proud of u-ctags project attracting a new hacker.

I don't know if I'll stay for long or contribute much to the project, but I'd like to at least try.

Though the required knowledge for developing u-ctags is only C and
standard C library

Not sure about those requirements, it seems to me there's a lot to learn before I can even grasp the ctags big picture and contribute something more valuable... (this PR shows exactly that, a C bug, didn't make tests or anything)

I would like you to add a prefix to the subject of commit

Done, was that what you wanted? Tell me if there's anything missing

@masatake masatake merged commit 1620c63 into universal-ctags:master Jan 13, 2018
@masatake
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants