Page MenuHomePhabricator

dougpuob (Douglas Chen)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 7 2018, 10:34 PM (97 w, 4 d)

Recent Activity

Yesterday

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Hi @njames93,

It's a smart idea, the rework for it is worth. There is a special case if lowercase name with Hungarian prefix, it possibly makes variable ambiguous, like the Case1. How about separating them and aNy_CasE with an underscore, like Case2 ?

// Case1
bool bRIGHT_LEVEL;     // UPPER_CASE
bool bRightLevel;      // CamelCase
bool bRight_Level;     // Camel_Snake_Case
bool baNy_CasE;        // aNy_CasE
bool bright_level;     // lower_case
bool brightLevel;      // camelBack
bool bright_Level;     // camel_Snake_Back
.....^^^^^^ <-- right? bright?

// Case2
bool bRIGHT_LEVEL;     // UPPER_CASE
bool bRightLevel;      // CamelCase
bool bRight_Level;     // Camel_Snake_Case
bool b_aNy_CasE;       // aNy_CasE
bool b_right_level;    // lower_case
bool b_rightLevel;     // camelBack
bool b_right_Level;    // camel_Snake_Back
.....^^^^^^^ <-- add an underscore

That still has hidden surprises. Maybe instead of a bool, an enum is used for controlling hungarian prefix (Off|On|...).
Can't think of a good name for the third option but it would do the inserting of '_' (bright_level ->b_right_level) or capitalising the first word of the identifier (brightLevel -> bRightLevel).

Tue, Oct 20, 4:38 PM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Is this diff been created incorrectly again?

Taking a step back, Is Hungarian notation really a case style, Seems to me its mainly about the prefix and a user may want DWORD dwUPPER_CASE, Right now there is no way of adopting that.
Maybe extend the options for hungarian type decls to

<Type>Case
<Type>Prefix
<Type>Suffix
<Type>HungarianPrefix

<Type>HungarianPrefix would likely be a bool and if enabled, it would be prepended to <Type>Prefix
I could see a situation like this

[Options]
// VariableCase: UPPER_CASE
// VariablePrefix: PRE_
// VariableSuffix: _POST
// VariableHungarianPrefix: true

DWORD MyVariable; -> DWORD dwPRE_MY_VARIABLE_POST;

This would give users full control of exactly how they want their declarations with no hidden surprises.

Granted this approach would require a little rework but it would satisfy more users.

Tue, Oct 20, 7:54 AM · Unknown Object (Project), Restricted Project

Tue, Oct 13

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Merged all commits as one single commit. (Trying to resolve "No such file or directory" error from the scripts/phabtalk/apply_patch2.py!_apply_diff() function)

Tue, Oct 13, 10:47 PM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Hi @aaron.ballman and @njames93,
I addressed your code review suggestions and supported Hungarian Notation prefix for decl of enum and class(option) at latest patch. Unfortunately, I encountered a problem that new patch failed on remote BuildBot, it showed the "No such file or directory" error message when it was trying to call apply_patch2.py!_apply_diff(). Do you have any idea what is going on? Do you suggest I create a new Diff(new diff id) for it ?

Tue, Oct 13, 6:20 PM · Unknown Object (Project), Restricted Project
dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Rename file name, "readability-identifier-naming-hungarian-notation-default.cpp" to "readability-identifier-naming-hungarian-notation.cpp".

Tue, Oct 13, 5:45 PM · Unknown Object (Project), Restricted Project
dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Merged with the master then do arc diff master --update D86671 again. (Trying to resolve "No such file or directory" error from the scripts/phabtalk/apply_patch2.py!_apply_diff() function)

Tue, Oct 13, 4:09 AM · Unknown Object (Project), Restricted Project

Sun, Oct 11

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Previous arc diff got problem(failed on builtbot), do it again with arc diff master --update D86671.

Sun, Oct 11, 6:40 AM · Unknown Object (Project), Restricted Project

Sat, Oct 10

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
  • Support to add Class prefix for Hungarian Notation.
  • Support to add Enum prefix for Hungarian Notation.
  • Support unsigned long long, ULONG, and HANDLE types and others.
  • Support options for Hungarian Notation in config file.
  • Added more test cases.
Sat, Oct 10, 11:11 PM · Unknown Object (Project), Restricted Project
dougpuob added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Sat, Oct 10, 9:46 PM · Unknown Object (Project), Restricted Project

Wed, Sep 30

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Not strictly relevant here, but this does open up the idea of enforcing the style where an enum constant is prefixed by the initials of the enum name.

Wed, Sep 30, 8:17 PM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Hi @aaron.ballman
About changing size_t nLength to cbLength. I searched MSVC folders with size_t, many names of variable start with n, or i in MFC related files. So I prefer to keep it starts with n. Another side to name starts with cb, I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR type.

I think the important point is that cb is used for APIs that specify a count of bytes, whereas i, n, and l are used for integers (there is no specific prefix for size_t that I'm aware of or that MSDN documents, so the lack of consistency there is not too surprising). That's why I mentioned that using a heuristic approach may allow you to identify the parameters that are intended to be a count of bytes so that you can use the more specific prefix if there's more specific contextual information available.

Wed, Sep 30, 9:57 AM · Unknown Object (Project), Restricted Project

Sep 20 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Hi @aaron.ballman
About changing size_t nLength to cbLength. I searched MSVC folders with size_t, many names of variable start with n, or i in MFC related files. So I prefer to keep it starts with n. Another side to name starts with cb, I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR type.

Sep 20 2020, 10:24 AM · Unknown Object (Project), Restricted Project

Sep 19 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Replied comments by @aaron.ballman

Sep 19 2020, 10:16 AM · Unknown Object (Project), Restricted Project

Sep 13 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Did you upload this incorrectly again, context is missing and seems to be a relative diff from a previous version of this patch?

Sorry for it, I think it's my bad. It is possible that I manually merged the last master(github) with changes then updated them both via web interface ...

Can I fix it if switch back to the base (14948a0) then merge all my changes, then update the diff again via web interface? Or do you have any better suggestion?

I am curious about how do you know this mistake? You got error messages with arc patch D86671 ?

The no context is easy to spot as phab says context not available. Its easy to find knowing that there is no mention of hungarian notation in the trunk version of IdentifierNamingCheck.cpp, yet there is mention of that in the before diff.

The way I do my patches is I create a branch from the current master. Then all commits go into that branch. When its time to update the PR I can just do a diff from <feature_branch> to <master>.
Though I do use arcanist for my patches

arc diff master

arcanist will check to see if the current branch has tags for PR and automatically update that PR. Otherwise it will create a new PR.
If it goes to create a new PR instead of updating an existing one you can pass update

arc diff master --update D86671
Sep 13 2020, 8:02 AM · Unknown Object (Project), Restricted Project

Sep 11 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Fixed crash on Windows when run regression test (llvm-lit for readability-identifier-naming.cpp file).

Sep 11 2020, 7:53 PM · Unknown Object (Project), Restricted Project

Sep 9 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
  • Fixed lint warnings and regression test failures on Windows.
Sep 9 2020, 6:43 AM · Unknown Object (Project), Restricted Project

Sep 8 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

This is a test with arc diff master --update D86671 command.

Sep 8 2020, 7:30 AM · Unknown Object (Project), Restricted Project

Sep 7 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Did you upload this incorrectly again, context is missing and seems to be a relative diff from a previous version of this patch?

Sep 7 2020, 9:28 AM · Unknown Object (Project), Restricted Project

Sep 5 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Addressed comments by @aaron.ballman

Sep 5 2020, 10:03 AM · Unknown Object (Project), Restricted Project

Sep 2 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Please no worry to give me your suggestions and feedback.

Sep 2 2020, 8:24 PM · Unknown Object (Project), Restricted Project

Sep 1 2020

dougpuob added a comment to D86930: [clang-format] Handle typename macros inside cast expressions.

I am a beginner to compiler, interesting in how to write Unit Test case for change so I ran it, but found difference with my expection.

Sep 1 2020, 5:31 PM · Restricted Project

Aug 31 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Improved suggestions of code review.

  1. Moved release notes to right place. [Eugene.Zelenko]
  2. Added new casting types to doc(readability-identifier-naming.rst) [Eugene.Zelenko]
  3. Moved partial code to a new function, IdentifierNamingCheck::getDeclTypeName(). [njames93]
Aug 31 2020, 6:52 AM · Unknown Object (Project), Restricted Project

Aug 30 2020

dougpuob added inline comments to D86847: [Bitcode] Add BITCODE_SIZE_BLOCK_ID to encode the size of the bitcode.
Aug 30 2020, 10:29 PM · Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

As Hungarian notation enforces prefixes as well as casing styles it would be wise to warn on cases where the style is Hungarian but a prefix has also been set.

Another issue is this current set up will let you define any decls style as hungarian, this doesn't make sense for most decl types.
Potentially some validation should happen for that too,

Aug 30 2020, 10:24 PM · Unknown Object (Project), Restricted Project
dougpuob added inline comments to D86847: [Bitcode] Add BITCODE_SIZE_BLOCK_ID to encode the size of the bitcode.
Aug 30 2020, 12:07 AM · Restricted Project

Aug 29 2020

dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

By the word, you must mention new option in documentation too.

Aug 29 2020, 9:14 PM · Unknown Object (Project), Restricted Project
dougpuob added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 29 2020, 8:39 PM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

I fixed all review suggestions from @aaron.ballman, @Eugene.Zelenko, and @njames93, Thank you. But I still can't sure I did everything correct.

Aug 29 2020, 2:08 PM · Unknown Object (Project), Restricted Project
dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Improved for suggestions of code review.

Aug 29 2020, 1:48 PM · Unknown Object (Project), Restricted Project

Aug 28 2020

dougpuob added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 28 2020, 9:21 AM · Unknown Object (Project), Restricted Project
dougpuob added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 28 2020, 9:14 AM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

It'll be good idea to add test case.

Aug 28 2020, 4:30 AM · Unknown Object (Project), Restricted Project
dougpuob added a comment to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Can you make sure you upload diffs with full context (-U99999). Or using arcanist it will be done automatically.

Make sure the diff is up to date with trunk

Remove any changes that aren't related to this patch, they just make this look noisy.

Aug 28 2020, 4:21 AM · Unknown Object (Project), Restricted Project

Aug 27 2020

dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Improved suggestions from code review and clang-tidy.

  1. Add keyword const to variables which checked via clang-tidy.
  2. Add log to clang-tools-extra/docs/ReleaseNotes.rst. [Suggestion from Eugene.Zelenko]
  3. Don't use auto with variables are not specified explicitly. [Suggestion from Eugene.Zelenko]
Aug 27 2020, 8:06 PM · Unknown Object (Project), Restricted Project
dougpuob updated the diff for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.

Fixed typos and add new Case Type, szHungarianNotation in doc.

Aug 27 2020, 6:02 AM · Unknown Object (Project), Restricted Project
dougpuob retitled D86671: [clang-tidy] Add new case type to check variables with Hungarian notation from Add new IdentifierNamingCheck::CaseType, CT_HungarianNotion, supporting naming check with Hungarian Notion. to [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 27 2020, 5:58 AM · Unknown Object (Project), Restricted Project

Aug 26 2020

dougpuob changed the visibility for D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 26 2020, 7:20 PM · Unknown Object (Project), Restricted Project
dougpuob requested review of D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Aug 26 2020, 7:00 PM · Unknown Object (Project), Restricted Project