[Refactor Xcode Objects] Enable generating per file '--help' compiler flag

[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating '--help'
as per file compiler flag for the files in the "Compiler Sources" in
"Build Phases".

BUG=614818
Committed: https://crrev.com/8fb7d9d9c9ddf8bd3d1448241fafa27416e1f756
Cr-Commit-Position: refs/heads/master@{#439913}

[liaoyuke, 2016-12-14 19:55:34]

Description was changed from

==========
[Refactor Xcode objects]: enable adding per file '-h' compiler flag

BUG=
==========

to

==========
[Refactor Xcode Objects] Enable adding per file '-h' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating '-h'
as per file compiler flag for the files in the "Compiler Sources" in "Build
Phases".

BUG=614818
==========

[liaoyuke, 2016-12-14 19:55:34]

liaoyuke@chromium.org changed reviewers:
+ justincohen@chromium.org, sdefresne@chromium.org

[liaoyuke, 2016-12-14 19:56:05]

Description was changed from

==========
[Refactor Xcode Objects] Enable adding per file '-h' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating '-h'
as per file compiler flag for the files in the "Compiler Sources" in "Build
Phases".

BUG=614818
==========

to

==========
[Refactor Xcode Objects] Enable adding per file '-h' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating '-h'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818
==========

[liaoyuke, 2016-12-14 19:56:44]

Hi Sylvain, Justin,

Please take a look.

Thank you very much!

[justincohen, 2016-12-14 20:00:17]

https://codereview.chromium.org/2574333002/diff/1/tools/gn/xcode_object.cc
File tools/gn/xcode_object.cc (right):

https://codereview.chromium.org/2574333002/diff/1/tools/gn/xcode_object.cc#ne...
tools/gn/xcode_object.cc:395: {"COMPILER_FLAGS", "-h"},
Is it -h or --help ?

[liaoyuke, 2016-12-14 20:29:24]

Addressed comments.

PTAL!

https://codereview.chromium.org/2574333002/diff/1/tools/gn/xcode_object.cc
File tools/gn/xcode_object.cc (right):

https://codereview.chromium.org/2574333002/diff/1/tools/gn/xcode_object.cc#ne...
tools/gn/xcode_object.cc:395: {"COMPILER_FLAGS", "-h"},
On 2016/12/14 20:00:17, justincohen wrote:
> Is it -h or --help ?

Thank you for catching it! I meant to type "-help". It seems that both "-help"
and "--help" would work, but I'd go with "--help" as this is the one used in
Clang documentation.

https://codereview.chromium.org/2574333002/diff/1/tools/gn/xcode_object.cc#ne...
tools/gn/xcode_object.cc:395: {"COMPILER_FLAGS", "-h"},
On 2016/12/14 20:00:17, justincohen wrote:
> Is it -h or --help ?

Done.

[liaoyuke, 2016-12-14 20:32:54]

Description was changed from

==========
[Refactor Xcode Objects] Enable adding per file '-h' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating '-h'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818
==========

to

==========
[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating
'--help'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818
==========

[sdefresne, 2016-12-17 00:07:27]

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_object.h
File tools/gn/xcode_object.h (right):

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_object.h...
tools/gn/xcode_object.h:156: enum HelpCompilerFlagEnabled { HELP_FLAG_ENABLED,
HELP_FLAG_DISABLED };
I don't really like the redundancy of the enumeration name and value and
variable.

What about moving the enum outside of the class, renaming it to CompilerFlags,
changing it to enum class (for proper scoping) and renaming the variable as
compiler_flags_.

Something like this:

enum class CompilerFlags {
  NONE,
  HELP,
};

class PBXBuildFile : public PBXObject {
 public:
  PBXBuildFile(const PBXFileReference* file_reference,
               const PBXSourcesBuildPhase* build_phase,
               CompilerFlags compiler_flags);
  ...
};

Then you can use it like this, later in the code:

  if (compiler_flags_ == CompilerFlags::HELP) {
    std::map<std::string, std::string> ...
  }

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:356: sources_for_indexing->AddSourceFile(
Please add a comment here explaining why you add the --help flag to the compiler
command-line. I know why (this is explained in your document), but future reader
of this code may wonder why. Could be as simple as saying that the file need to
be know to Xcode for proper indexing and for discovery of tests function for
XCTest but that the compilation is done via ninja and thus must be disabled in
Xcode via this hack.

[liaoyuke, 2016-12-19 08:02:40]

PTAL!

I know you are pretty busy with upstreaming. So, no rush, take your time to
review.

Thank you very much!

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_object.h
File tools/gn/xcode_object.h (right):

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_object.h...
tools/gn/xcode_object.h:156: enum HelpCompilerFlagEnabled { HELP_FLAG_ENABLED,
HELP_FLAG_DISABLED };
On 2016/12/17 00:07:27, sdefresne wrote:
> I don't really like the redundancy of the enumeration name and value and
> variable.
> 
> What about moving the enum outside of the class, renaming it to CompilerFlags,
> changing it to enum class (for proper scoping) and renaming the variable as
> compiler_flags_.
> 
> Something like this:
> 
> enum class CompilerFlags {
>   NONE,
>   HELP,
> };
> 
> class PBXBuildFile : public PBXObject {
>  public:
>   PBXBuildFile(const PBXFileReference* file_reference,
>                const PBXSourcesBuildPhase* build_phase,
>                CompilerFlags compiler_flags);
>   ...
> };
> 
> Then you can use it like this, later in the code:
> 
>   if (compiler_flags_ == CompilerFlags::HELP) {
>     std::map<std::string, std::string> ...
>   }

Thank you for explaining the details. This is a much better design and it would
be more flexible if one needs to add other flags.

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574333002/diff/40001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:356: sources_for_indexing->AddSourceFile(
On 2016/12/17 00:07:27, sdefresne wrote:
> Please add a comment here explaining why you add the --help flag to the
compiler
> command-line. I know why (this is explained in your document), but future
reader
> of this code may wonder why. Could be as simple as saying that the file need
to
> be know to Xcode for proper indexing and for discovery of tests function for
> XCTest but that the compilation is done via ninja and thus must be disabled in
> Xcode via this hack.

Ack. Here we are only adding files to "sources", but will keep in mind to add
the comment later.

[sdefresne, 2016-12-19 08:05:36]

lgtm with nit

You'll want dpranke or brettw to review as OWNERS.

https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h
File tools/gn/xcode_object.h (right):

https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h...
tools/gn/xcode_object.h:154: enum class CompilerFlags {
nit: move at the top of the file with other enums

[liaoyuke, 2016-12-19 08:15:36]

liaoyuke@chromium.org changed reviewers:
+ dpranke@chromium.org

[liaoyuke, 2016-12-19 08:17:42]

Thank you for reviewing.

Hello Dirk,

Could you please take a look for owner's approval?

Thank you very much!

https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h
File tools/gn/xcode_object.h (right):

https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h...
tools/gn/xcode_object.h:154: enum class CompilerFlags {
On 2016/12/19 08:05:36, sdefresne wrote:
> nit: move at the top of the file with other enums

Done.

[liaoyuke, 2016-12-20 17:25:17]

On 2016/12/19 08:17:42, liaoyuke wrote:
> Thank you for reviewing.
> 
> Hello Dirk,
> 
> Could you please take a look for owner's approval?
> 
> Thank you very much!
> 
> https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h
> File tools/gn/xcode_object.h (right):
> 
>
https://codereview.chromium.org/2574333002/diff/60001/tools/gn/xcode_object.h...
> tools/gn/xcode_object.h:154: enum class CompilerFlags {
> On 2016/12/19 08:05:36, sdefresne wrote:
> > nit: move at the top of the file with other enums
> 
> Done.

ping?

[Dirk Pranke, 2016-12-20 21:26:00]

lgtm, sorry for the delay.

[liaoyuke, 2016-12-20 21:58:36]

Thank you for reviewing! :)

On Tue, Dec 20, 2016 at 1:26 PM <dpranke@chromium.org> wrote:

lgtm, sorry for the delay.

https://codereview.chromium.org/2574333002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[liaoyuke, 2016-12-20 22:55:34]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2016-12-20 22:55:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2574333002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-20 22:56:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 23:12:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482274534004220,
"parent_rev": "5f6c1d7425be7c0cd76a0e47ad83051d2914a2c7", "commit_rev":
"1374b22fb775128c559ace69dac3aaf47c7933d0"}

[commit-bot: I haz the power, 2016-12-20 23:12:54]

Description was changed from

==========
[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating
'--help'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818
==========

to

==========
[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating
'--help'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818

Review-Url: https://codereview.chromium.org/2574333002
==========

[commit-bot: I haz the power, 2016-12-20 23:12:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-20 23:15:54]

Description was changed from

==========
[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating
'--help'
as per file compiler flag for the files in the "Compiler Sources" in 
"Build Phases".

BUG=614818

Review-Url: https://codereview.chromium.org/2574333002
==========

to

==========
[Refactor Xcode Objects] Enable adding per file '--help' compiler flag.

This CL refactors xcode_object.h and xcode_object.cc to enable generating
'--help'
as per file compiler flag for the files in the "Compiler Sources" in
"Build Phases".

BUG=614818

Committed: https://crrev.com/8fb7d9d9c9ddf8bd3d1448241fafa27416e1f756
Cr-Commit-Position: refs/heads/master@{#439913}
==========

[commit-bot: I haz the power, 2016-12-20 23:15:55]

Patchset 7 (id:??) landed as
https://crrev.com/8fb7d9d9c9ddf8bd3d1448241fafa27416e1f756
Cr-Commit-Position: refs/heads/master@{#439913}