Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Allow '!' char in D parameter lists

  • Loading branch information...
commit f0f3fc83ad8a9ae54896422fa9668b31f1e3f6bf 1 parent 8d26450
@ntrel ntrel authored
Showing with 6 additions and 0 deletions.
  1. +6 −0 tagmanager/ctags/c.c
View
6 tagmanager/ctags/c.c
@@ -2363,6 +2363,12 @@ static int parseParens (statementInfo *const st, parenInfo *const info)
{
int c = skipToNonWhite ();
+ if (isLanguage(Lang_d) && c == '!')
+ { /* template instantiation */
+ info->isNameCandidate = FALSE;
+ info->isKnrParamList = FALSE;
+ }
+ else
switch (c)

shouldnt the following switch be indented? patch size would explode, yes, but still...

@codebrainz Owner

@ntrel here's another way we could do it but I don't really know D so I didn't test it:
https://gist.github.com/3173858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
{
case '&':

14 comments on commit f0f3fc8

@ntrel
Owner

@stefanct: indenting might cause conflicts when merging ctags.
@codebrainz: that looks less clean and more bug-prone.

@codebrainz
Owner

@ntrel more bug prone than a non-indented 100 line switch block inside a single-line else block with no braces? I'd say it looks far more clean and equally as (future) bug-prone :)

@ntrel
Owner

Now indented in 14daf92. To be honest the diff noise was the reason I didn't do it, but I should have. Merging with a good merge tool like meld allows ignoring space changes anyway.

@codebrainz: It's best to only change the code path for D here, not all languages.

@codebrainz
Owner

@ntrel yeah either way is OK. It was just something about putting the least likely case as the very first thing that gets tested for every character for every language first. I guess maybe a G_UNLIKELY would be useful here if it is indeed a tight loop like it appears to be.

@ntrel
Owner

@codebrainz CTags does not depend on GLib, so we should avoid using GLib symbols unless necessary. IMO micro-optimizing Geany is a waste of time.

@codebrainz
Owner

@ntrel Just for an idea, the D-only if condition gets tried first ~10,000 times just opening Geany's C and H files (before even triggering the "real-time" buffer parsing on each keypresses/250ms timer). I'd say it's debatable whether re-structuring/optimizing is worthwhile, but it's definitively fugly from a style/readability perspective (IMO). Anyway, I don't actually care that much so I'll stop annoying you about it :) Heh

@ntrel
Owner

You're talking about micro-optimizing parameter list character handling. There are doubtless better places to use likely/unlikely macros in CTags, but thankfully the CTags maintainer understands that adding these is not worthwhile.

"definitively ... IMO" - contradiction in terms there.

@codebrainz
Owner

I mean "In my opinion, I definitively consider it ugly", not "without question for everyone it's definitively ugly, IMO", sorry for not being clear :) I added the (IMO) after re-reading it in case maybe there were some people that thought it was not ugly to put a hack that will virtually never test true up front before the "normal" code. As far as optimizing, I don't really care about the perfomance, but if it says "unlikely" right there in the hack, readers will know that even though it's the first test that gets hit each time, it's very unlikely to actually test true and for the "real" code they should read the else block. A comment would do just as good.

Edit:
Just in case it's not clear; I'm just bikeshedding. I don't think it's a big deal either way, nor do I expect you to change it...just talkin' programmin' :)

@ntrel
Owner

The if test is clear enough. It really doesn't need explanation.

It is a good idea to put a short branch of an if statement before a really long branch, so that the test is near the code it relates to.

@codebrainz
Owner

Yeah, I didn't mean to imply we should invert the if test and move the body down at the bottom away from the test, that's for sure a bad idea. I just think it'd be better to integrate the test within the existing control structure (switch) like all the other logic. My original proposal was kind of hackish because it fell through to the default case, which is generally stupid, but what about something like this (untested):

            default:
            {
                if (isident1 (c))
                {
                    if (++identifierCount > 1)
                        info->isKnrParamList = FALSE;
                    readIdentifier (token, c);
                    if (isType (token, TOKEN_NAME)  &&  info->isNameCandidate)
                        token->type = TOKEN_PAREN_NAME;
                    else if (isType (token, TOKEN_KEYWORD))
                    {
                        info->isKnrParamList = FALSE;
                        info->isNameCandidate = FALSE;
                    }
                }
                else if (isLanguage(Lang_d) && c == '!')
                {   /* template instantiation */
                    info->isNameCandidate = FALSE;
                    info->isKnrParamList = FALSE;
                }
                else
                {
                    info->isParamList     = FALSE;
                    info->isKnrParamList  = FALSE;
                    info->isNameCandidate = FALSE;
                    info->invalidContents = TRUE;
                }
                break;
            }

That's just the "default" case of the switch block of that function with your new test added to it. As a bonus it probably would make it easier to meld with upstream too.

@ntrel
Owner

Maybe, but it breaks if upstream adds a case '!' and we merge it.

@codebrainz
Owner

Lots of stuff breaks in c.c if upstream changes it :)

@ntrel
Owner

It's OK with me if you want to test and commit that change.

@codebrainz
Owner

Done in commit e76a35c (and 63249c7).

Please sign in to comment.
Something went wrong with that request. Please try again.