Skip to content

C#: Rename and restructure control flow graph entities#140

Merged
calumgrant merged 3 commits into
github:rc/1.18from
hvitved:csharp/cfg/renaming
Sep 5, 2018
Merged

C#: Rename and restructure control flow graph entities#140
calumgrant merged 3 commits into
github:rc/1.18from
hvitved:csharp/cfg/renaming

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented Sep 3, 2018

Follow a naming structure similar to the data flow library:

  • ControlFlowNode -> ControlFlow::Node.
  • CallableEntryNode -> ControlFlow::Nodes::EntryNode.
  • CallableExitNode -> ControlFlow::Nodes::ExitNode.
  • ControlFlowEdgeType -> ControlFlow::SuccessorType.
  • ControlFlowEdgeSuccessor -> ControlFlow::SuccessorTypes::NormalSuccessor.
  • ControlFlowEdgeConditional -> ControlFlow::SuccessorTypes::ConditionalSuccessor.
  • ControlFlowEdgeBoolean -> ControlFlow::SuccessorTypes::BooleanSuccessor.
  • ControlFlowEdgeNullness -> ControlFlow::SuccessorTypes::NullnessSuccessor.
  • ControlFlowEdgeMatching -> ControlFlow::SuccessorTypes::MatchingSuccessor.
  • ControlFlowEdgeEmptiness -> ControlFlow::SuccessorTypes::EmptinessSuccessor.
  • ControlFlowEdgeReturn -> ControlFlow::SuccessorTypes::ReturnSuccessor.
  • ControlFlowEdgeBreak -> ControlFlow::SuccessorTypes::BreakSuccessor.
  • ControlFlowEdgeContinue -> ControlFlow::SuccessorTypes::ContinueSuccessor.
  • ControlFlowEdgeGotoLabel -> ControlFlow::SuccessorTypes::GotoLabelSuccessor.
  • ControlFlowEdgeGotoCase -> ControlFlow::SuccessorTypes::GotoCaseSuccessor.
  • ControlFlowEdgeGotoDefault -> ControlFlow::SuccessorTypes::GotoDefaultSuccessor.
  • ControlFlowEdgeException -> ControlFlow::SuccessorTypes::ExceptionSuccessor.

@hvitved hvitved added the C# label Sep 3, 2018
@hvitved hvitved added this to the 1.18 milestone Sep 3, 2018
@hvitved hvitved requested a review from calumgrant September 3, 2018 12:22
@hvitved hvitved requested a review from a team as a code owner September 3, 2018 12:22
@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented Sep 3, 2018

For some reason, github's "Hide whitespace changes" does not work well with the changes in ControlFlowGraph.qll: viewing the changes in gitk and selecting "Ignore space changes" works much better.

@hvitved hvitved force-pushed the csharp/cfg/renaming branch 2 times, most recently from 375aff4 to 84da5a3 Compare September 3, 2018 19:10
@calumgrant
Copy link
Copy Markdown
Contributor

calumgrant commented Sep 4, 2018

The change itself looks very reasonable, however I question whether this needs to go into the RC branch. We should also create some deprecated aliases for the old symbols (particularly, ControlFlowNode) before removing them completely, and we should have a change note.

@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented Sep 4, 2018

The change itself looks very reasonable, however I question whether this needs to go into the RC branch. We should also create some deprecated aliases for the old symbols (particularly, ControlFlowNode) before removing them completely, and we should have a change note.

I did add entities for backwards compatibility: https://github.com/Semmle/ql/blob/84da5a36090b01c8f24d96c5ab36c6cfb284041f/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll#L3155

Sure, I can add a change note.

I targeted the RC branch because I also removed all compiler warnings on this PR.

@hvitved hvitved requested a review from shati-patel September 4, 2018 14:14
shati-patel
shati-patel reviewed Sep 4, 2018
View reviewed changes
Comment thread change-notes/1.18/analysis-csharp.md Outdated
- `CallableExitNode` has been renamed to `ControlFlow::Nodes::ExitNode`.
- `ControlFlowEdgeType` has been renamed to `ControlFlow::SuccessorType`.
- `ControlFlowEdgeSuccessor` has been renamed to `ControlFlow::SuccessorTypes::NormalSuccessor`.
- `ControlFlowEdgeConditional has been renamed to ControlFlow::SuccessorTypes::ConditionalSuccessor`.
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is missing a few backticks (before "has" and after "to").

Otherwise the change note LGTM.

@calumgrant
Copy link
Copy Markdown
Contributor

calumgrant commented Sep 4, 2018

Sorry Tom, I hadn't seen the aliases when I searched for them due to the large diff being hidden.

@calumgrant
Copy link
Copy Markdown
Contributor

calumgrant commented Sep 4, 2018 •
edited
Loading

There seems to be repetition in SuccessorTypes::BooleanSuccessor. Would it be any clearer to name them SuccessorTypes::Boolean etc instead?

@hvitved
Copy link
Copy Markdown
Contributor Author

hvitved commented Sep 4, 2018

There seems to be repetition in SuccessorTypes::BooleanSuccessor. Would it be any clearer to name them SuccessorTypes::Boolean etc instead?

I prefer the naming as is; the nested SuccessorTypes module is mostly there to limit the number of entities inside the ControlFlow module.

calumgrant
calumgrant previously approved these changes Sep 5, 2018
View reviewed changes
@hvitved
C#: Rename and restructure control flow graph entities
42faabc 
Follow a naming structure similar to the data flow library:

- `ControlFlowNode` -> `ControlFlow::Node`.
- `CallableEntryNode` -> `ControlFlow::Nodes::EntryNode`.
- `CallableExitNode` -> `ControlFlow::Nodes::ExitNode`.
- `ControlFlowEdgeType` -> `ControlFlow::SuccessorType`.
- `ControlFlowEdgeSuccessor` -> `ControlFlow::SuccessorTypes::NormalSuccessor`.
- `ControlFlowEdgeConditional -> ControlFlow::SuccessorTypes::ConditionalSuccessor`.
- `ControlFlowEdgeBoolean` -> `ControlFlow::SuccessorTypes::BooleanSuccessor`.
- `ControlFlowEdgeNullness` -> `ControlFlow::SuccessorTypes::NullnessSuccessor`.
- `ControlFlowEdgeMatching` -> `ControlFlow::SuccessorTypes::MatchingSuccessor`.
- `ControlFlowEdgeEmptiness` -> `ControlFlow::SuccessorTypes::EmptinessSuccessor`.
- `ControlFlowEdgeReturn` -> `ControlFlow::SuccessorTypes::ReturnSuccessor`.
- `ControlFlowEdgeBreak` -> `ControlFlow::SuccessorTypes::BreakSuccessor`.
- `ControlFlowEdgeContinue` -> `ControlFlow::SuccessorTypes::ContinueSuccessor`.
- `ControlFlowEdgeGotoLabel` -> `ControlFlow::SuccessorTypes::GotoLabelSuccessor`.
- `ControlFlowEdgeGotoCase` -> `ControlFlow::SuccessorTypes::GotoCaseSuccessor`.
- `ControlFlowEdgeGotoDefault` -> `ControlFlow::SuccessorTypes::GotoDefaultSuccessor`.
- `ControlFlowEdgeException` -> `ControlFlow::SuccessorTypes::ExceptionSuccessor`
@hvitved
C#: Add change note
82b2c00
@hvitved
C#: Fix typos in change note
061b3d4
@hvitved hvitved force-pushed the csharp/cfg/renaming branch from 40b0d42 to 061b3d4 Compare September 5, 2018 12:21
@calumgrant calumgrant merged commit 727ab94 into github:rc/1.18 Sep 5, 2018
@hvitved hvitved deleted the csharp/cfg/renaming branch September 6, 2018 06:33
aibaars added a commit that referenced this pull request Oct 14, 2021
@aibaars
Merge pull request #140 from github/aibaars/namespace
a7408dd 
AST: introduce 'Namespace' as super class of Class/Module
smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021
@igfoo
Merge pull request github#140 from github/igfoo/enum
52aec00 
Kotlin: Allow comments on enum entries
MathiasVP added a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025
@MathiasVP
Merge pull request github#140 from microsoft/powershell-storestep-avo…
5b5f6ec 
…id-cp

PS: Remove accidental CP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shati-patel shati-patel shati-patel left review comments

+1 more reviewer

@calumgrant calumgrant calumgrant approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C# documentation

Projects

None yet

Milestone

1.18

Development

Successfully merging this pull request may close these issues.

4 participants

@hvitved @calumgrant @shati-patel @felicitymay