Page MenuHomePhabricator

[clang,flang] Add help text for -fsyntax-only
ClosedPublic

Authored by alexiprof on Aug 12 2022, 2:16 PM.

Details

Summary

Fix for the problem with displaying options -fsyntax-only in clang and flang-new in help
Fix https://github.com/llvm/llvm-project/issues/57033

Before:

$ clang  -help | grep syntax
 -objcmt-migrate-property-dot-syntax
        Enable migration of setter/getter messages to property-dot syntax

After:

$ clang -help | grep syntax
 -fsyntax-only           Run the preprocessor, parser and semantic analysis stages
 -objcmt-migrate-property-dot-syntax
        Enable migration of setter/getter messages to property-dot syntax

Diff Detail

Event Timeline

alexiprof created this revision.Aug 12 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
alexiprof requested review of this revision.Aug 12 2022, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 2:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
alexiprof edited the summary of this revision. (Show Details)Aug 12 2022, 3:03 PM
alexiprof added a project: Restricted Project.
vzakhari accepted this revision.Aug 12 2022, 3:18 PM
vzakhari added a subscriber: vzakhari.

LGTM.

This revision is now accepted and ready to land.Aug 12 2022, 3:18 PM

The CI failed since the test cases in flang driver have new output.

This is commit fix flang unit tests in using help command

Fix revision

Differential Revision: https://reviews.llvm.org/D131808

This is primarily a Clang change, so added some Clang reviewers. I will review shortly - thanks for taking this on!

Thanks, mostly makes sense! https://github.com/llvm/llvm-project/issues/57033 mentions -O{n} as well :) Could you fix that too? More comments inline.

CI is still failing :( Are you able to re-produce that? (I'm traveling atm, so can't check).

clang/include/clang/Driver/Options.td
2803–2805

Note that:

$ gcc --help=common | grep syntax
  -fsyntax-only               Check for syntax errors, then stop.

and (from Clang docs

Run the preprocessor, parser and type checking stages.

For consistency sake, I would replace "Syntax-check only"" with "Run the preprocessor, parser and type checking stages.". Technically, there's no separate preprocessor in Flang, but that's IMO tangential to this.

3219–3220

Unrelated change?

alexiprof added a comment.EditedAug 13 2022, 2:27 AM

Thanks, mostly makes sense! https://github.com/llvm/llvm-project/issues/57033 mentions -O{n} as well :) Could you fix that too? More comments inline.

CI is still failing :( Are you able to re-produce that? (I'm traveling atm, so can't check).

@awarzynski thanks for the feedback
Yes, if is valid phrase for flang and clang "Run the preprocessor, parser and type checking stages", I'll put it on.

in gcc optimization level displayed:

-O<number>                  Set optimization level to <number>.

I can add this to the list, but I think it's better to make it a separate revision

alexiprof edited the summary of this revision. (Show Details)Aug 13 2022, 3:49 AM
alexiprof updated this revision to Diff 452405.Aug 13 2022, 4:03 AM

Set new text hint for option fsyntax-only

alexiprof edited the summary of this revision. (Show Details)Aug 13 2022, 4:11 AM
alexiprof added inline comments.Aug 13 2022, 4:21 AM
clang/include/clang/Driver/Options.td
2803–2805

Yes, i am replace text

3219–3220

yes, mini fix format.
If it bothers you, I can remove it

alexiprof updated this revision to Diff 452429.Aug 13 2022, 8:37 AM

this pull request contains a fix for the problem with displaying options -fsyntax_only in clang and fland-new in help
https://github.com/llvm/llvm-project/issues/57033

Before:

$ clang  -help | grep syntax
 -objcmt-migrate-property-dot-syntax
        Enable migration of setter/getter messages to property-dot syntax

After:

$ clang -help | grep syntax
 -fsyntax-only           Run the preprocessor, parser and type checking stages
 -objcmt-migrate-property-dot-syntax
        Enable migration of setter/getter messages to property-dot syntax

Differential Revision: https://reviews.llvm.org/D131808

alexiprof updated this revision to Diff 452431.Aug 13 2022, 8:44 AM

remove Unrelated change

alexiprof marked 2 inline comments as done.Aug 13 2022, 9:21 AM
MaskRay accepted this revision.Aug 13 2022, 9:33 PM

Subject: [clang,flang] Add missing options fsyntax-only in help

Prefer -fsyntax-only to fsyntax-only since the former is what a user writes.

this pull request contains a fix for the problem with displaying options -fsyntax_only in clang and fland-new in help

typo: -fsyntax-only

"this pull request contains a fix" is verbose. Just use an imperative sentence: "Fix ..."

clang/include/clang/Driver/Options.td
2803–2805

semantic analysis is probably better than "type checking".

https://github.com/llvm/llvm-project/issues/57033

Use Fix https://github.com/llvm/llvm-project/issues/57033 or Fixes https://github.com/llvm/llvm-project/issues/57033 or (I usually prefer this for non-bugs but still some issues worth addressing) Close ..., then the issue will be automatically closed when you push the commit to origin/main

alexiprof edited the summary of this revision. (Show Details)Aug 13 2022, 10:36 PM
alexiprof edited the summary of this revision. (Show Details)

I do not have commit access, i do not have merge :-(

To be clear, some issues should be addressed before someone lands this.

alexiprof edited the summary of this revision. (Show Details)Aug 14 2022, 12:05 AM
alexiprof edited the summary of this revision. (Show Details)
alexiprof updated this revision to Diff 452487.EditedAug 14 2022, 12:07 AM

replace text hint

alexiprof marked an inline comment as done.Aug 14 2022, 1:23 AM
alexiprof requested review of this revision.Aug 15 2022, 2:05 AM
alexiprof accepted this revision.Aug 15 2022, 2:47 AM
This revision is now accepted and ready to land.Aug 15 2022, 2:47 AM
alexiprof closed this revision.Aug 15 2022, 2:47 AM
alexiprof reopened this revision.
This revision is now accepted and ready to land.Aug 15 2022, 2:47 AM

I can't merge the changes((

Could someone land this patch please?
Alexander Malkov <alexmal289@gmail.com>

awarzynski accepted this revision.Aug 16 2022, 9:36 AM

LGTM, thanks for the fix!

You may want to update to title to better reflect the contents (e.g. “Add help text for -fsyntax-only”). While the fact that it fixes https://github.com/llvm/llvm-project/issues/57033 motivated this change, the actual implementation does a bit more than just fixing that.

I can land this for you once I can access stable internet. Sorry for the delay.

alexiprof retitled this revision from [clang,flang] Add missing options fsyntax-only in help to [clang,flang] Add help text for -fsyntax-only.Aug 16 2022, 1:12 PM
This revision was automatically updated to reflect the committed changes.

Could someone land this patch please?
Alexander Malkov <alexmal289@gmail.com>

Was landed with the email id contained in the patch (Author: Alexander Malkov <alexiprof@yandex-team.ru>). Hope that is OK. May be you can update to the gmail id if that is what you prefer in your phabricator account.