Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12845013:
Adding _Check* function for invalid OS_MACROs in src/PRESUBMIT.py (Closed)
|
Created:
7 years, 9 months ago by Dan Beam Modified:
7 years, 9 months ago CC:
chromium-reviews, darin (slow to review), Mark Mentovai, scottmg, groby-ooo-7-16 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding _Check* function for invalid OS_MACROs in src/PRESUBMIT.py
R=maruel@chromium.org
BUG=none
CONTEXT=http://goo.gl/hrlj1
TEST=PRESUBMIT_test.py and less OS_MAXOSX!
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190693
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 3
Patch Set 4 : . #Patch Set 5 : . #
Total comments: 7
Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : . #
Total comments: 2
Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 4
Patch Set 12 : . #Messages
Total messages: 22 (0 generated)
Just FYI, as I'm not sure what the standards of evidence are for presubmit checks. Thanks for looking into this, though. https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py#newcode832 PRESUBMIT.py:832: pattern = input_api.re.compile(r'\b(OS_[A-Z]{3,})\b') Have you run this over the existing codebase? I haven't actually checked for cases where we have OS_SOMETHING not inside of defined(OS_SOMETHING). If it's not in a defined() clause, then it's completely incorrect anyhow, so I'm not sure whether I care about warning on those cases, but I do think it's worthwhile to not have people routinely overriding the presubmit because of false positives.
https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py#newcode832 PRESUBMIT.py:832: pattern = input_api.re.compile(r'\b(OS_[A-Z]{3,})\b') On 2013/03/21 04:20:00, shess wrote: > Have you run this over the existing codebase? I haven't actually checked for > cases where we have OS_SOMETHING not inside of defined(OS_SOMETHING). If it's > not in a defined() clause, then it's completely incorrect anyhow, so I'm not > sure whether I care about warning on those cases, but I do think it's worthwhile > to not have people routinely overriding the presubmit because of false > positives. I think people putting OS_BLAH in comments is equally bad at propagating the wrong names as #if defined(OS_BLAH) which is why I'm intentionally not looking for ^#(?:el|end)?if(?:n?def)? And no, I haven't run it on the whole codebase (yet).
shess@: alright, check out the updated patch, it narrows the scope a bit (mainly out of desire for me not to have 100s of bugs filed in my direction)
On 2013/03/21 04:42:15, Dan Beam wrote: > shess@: alright, check out the updated patch, it narrows the scope a bit (mainly > out of desire for me not to have 100s of bugs filed in my direction) Your change uses regular expression options significantly beyond what I can read when sober. But I get the impression that it tries to handle #ifdef or #ifndef type things - our code only allows #if defined() in cases like this. I still would hold with matching defined(), but I'm happy to let maruel be the decider. Another reason I was thinking of matching defined() rather than #something is because I noticed at least one case where the #if was wrapped to a second line. I don't think trying to regexp your way to a preprocessor is a good idea. https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/12845013/diff/4001/PRESUBMIT.py#newcode832 PRESUBMIT.py:832: pattern = input_api.re.compile(r'\b(OS_[A-Z]{3,})\b') On 2013/03/21 04:29:09, Dan Beam wrote: > On 2013/03/21 04:20:00, shess wrote: > > Have you run this over the existing codebase? I haven't actually checked for > > cases where we have OS_SOMETHING not inside of defined(OS_SOMETHING). If it's > > not in a defined() clause, then it's completely incorrect anyhow, so I'm not > > sure whether I care about warning on those cases, but I do think it's > worthwhile > > to not have people routinely overriding the presubmit because of false > > positives. > > I think people putting OS_BLAH in comments is equally bad at propagating the > wrong names as > > #if defined(OS_BLAH) I can see both ways, but if the comment doesn't contain "defined(OS_BLAH)", then they cannot easily copy/paste it to propagate the problem. And even if they copy/paste and add defined(), the presubmit will catch them on it.
should we be fixing these, then? https://gist.github.com/danbeam/97416bccd7a6c342a8dc
On 2013/03/21 15:19:17, Dan Beam wrote: > should we be fixing these, then? > https://gist.github.com/danbeam/97416bccd7a6c342a8dc and... CL - https://codereview.chromium.org/12545059/
https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:831: starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)') I agree with Scott the regexp is hard to read and ... it's wrong. Both following lines are valid preprocessor conditions: #if FOO # if FOO Note that you do a match but do not use ^. In practice it's fine so you match the first line, even if it's not obvious, but you don't match the second one. So for example, it'd match lines like: // There's #ifdef here but it's a comment So the name starts_with is confusing.
https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:362: pattern = input_api.re.compile(r'^#pragma\s+once', # pragma ? https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:831: starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)') On 2013/03/22 12:41:15, Marc-Antoine Ruel wrote: > I agree with Scott the regexp is hard to read and ... it's wrong. Both following > lines are valid preprocessor conditions: > #if FOO > # if FOO This will never be "right" unless it's a real C compiler. See numerous other places in this file that are partially parsing C. Do you want me to update with \s*#\s*? > > Note that you do a match but do not use ^. In practice it's fine so you match > the first line, even if it's not obvious, but you don't match the second one. So > for example, it'd match lines like: > // There's #ifdef here but it's a comment > > So the name starts_with is confusing. > > so you'd rather me add a ^ to the regexp and/or change to input_api.re.search()?
https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:831: starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)') On 2013/03/22 18:29:02, Dan Beam wrote: > On 2013/03/22 12:41:15, Marc-Antoine Ruel wrote: > > I agree with Scott the regexp is hard to read and ... it's wrong. Both > following > > lines are valid preprocessor conditions: > > #if FOO > > # if FOO > > This will never be "right" unless it's a real C compiler. See numerous other > places in this file that are partially parsing C. > > Do you want me to update with \s*#\s*? shess@ also recommended only looking defined(OS_NAME), would that be better? > > > > > Note that you do a match but do not use ^. In practice it's fine so you match > > the first line, even if it's not obvious, but you don't match the second one. > So > > for example, it'd match lines like: > > // There's #ifdef here but it's a comment > > > > So the name starts_with is confusing. > > > > > > so you'd rather me add a ^ to the regexp and/or change to input_api.re.search()?
https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:362: pattern = input_api.re.compile(r'^#pragma\s+once', On 2013/03/22 18:29:02, Dan Beam wrote: > # pragma ? DID I WRITE THIS CODE? I HOPE NOT! https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:831: starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)') On 2013/03/22 18:29:02, Dan Beam wrote: > This will never be "right" unless it's a real C compiler. See numerous other > places in this file that are partially parsing C. > > Do you want me to update with \s*#\s*? That's true, I just want to make sure the _intent_ is explicit and readable. And on that point, the intent is not well communicated because the variable name doesn't match what the regexp does. I don't have much opinion otherwise.
https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/11001/PRESUBMIT.py#newco... PRESUBMIT.py:831: starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)') On 2013/03/22 12:41:15, Marc-Antoine Ruel wrote: > I agree with Scott the regexp is hard to read and ... it's wrong. Both following > lines are valid preprocessor conditions: > #if FOO > # if FOO Fixed this and added test cases. > > Note that you do a match but do not use ^. In practice it's fine so you match > the first line, even if it's not obvious, but you don't match the second one. So > for example, it'd match lines like: > // There's #ifdef here but it's a comment > > So the name starts_with is confusing. > >
On 2013/03/21 16:01:42, Dan Beam wrote: > On 2013/03/21 15:19:17, Dan Beam wrote: > > should we be fixing these, then? > > https://gist.github.com/danbeam/97416bccd7a6c342a8dc > > and... CL - https://codereview.chromium.org/12545059/ FYI: this has landed so we can now key off of defined() for now (and maybe write a presubmit if we agree to add #ifdef -> #if defined() to the style guide)
ping
LGTM. I probably wouldn't even bother with the # check, the whole sequence is pretty unlikely to be in regular code (and not be referring to preprocessing), but I can see why you have it in there. https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py#newco... PRESUBMIT.py:854: 'src/PRESUBMIT.py or update your code.', bad_macros)] Do most presubmit failures suggest how to work around the failure? There are very few people indeed who should be adding new accepted defines.
https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py#newco... PRESUBMIT.py:854: 'src/PRESUBMIT.py or update your code.', bad_macros)] On 2013/03/26 02:45:46, shess wrote: > Do most presubmit failures suggest how to work around the failure? There are > very few people indeed who should be adding new accepted defines. I added a crappy suggest function. I could just list all the valid OSes (or make a fancier levenshtein distance calculation), but this seemed to be the right amount of work.
On 2013/03/26 03:46:03, Dan Beam wrote: > https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py > File PRESUBMIT.py (right): > > https://chromiumcodereview.appspot.com/12845013/diff/24001/PRESUBMIT.py#newco... > PRESUBMIT.py:854: 'src/PRESUBMIT.py or update your code.', bad_macros)] > On 2013/03/26 02:45:46, shess wrote: > > Do most presubmit failures suggest how to work around the failure? There are > > very few people indeed who should be adding new accepted defines. > > I added a crappy suggest function. I could just list all the valid OSes (or make > a fancier levenshtein distance calculation), but this seemed to be the right > amount of work. Your "crappy" suggest function is amazing! I actually didn't mention that because I thought users needed more help, I'd have just left the user to grep around. I hope that most cases will be "Oh, duh, OS_MAXOSX, hahaha!", and not genuine confusion. For those that _are_ genuine confusion, editing presubmit.py is definitely not the right way to go. Still LGTM.
https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py#newco... PRESUBMIT.py:842: except: except KeyError: https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py#newco... PRESUBMIT.py:871: return [] if not bad_macros else [output_api.PresubmitError( I think the embedded condition make the thing slightly unreadable because the rhs is 3 lines long.
https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py File PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py#newco... PRESUBMIT.py:842: except: On 2013/03/26 13:30:51, Marc-Antoine Ruel wrote: > except KeyError: Done. https://chromiumcodereview.appspot.com/12845013/diff/34001/PRESUBMIT.py#newco... PRESUBMIT.py:871: return [] if not bad_macros else [output_api.PresubmitError( On 2013/03/26 13:30:51, Marc-Antoine Ruel wrote: > I think the embedded condition make the thing slightly unreadable because the > rhs is 3 lines long. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/12845013/38001
Message was sent while issue was closed.
Change committed as 190693 |
