Page MenuHomePhabricator

[clang-tidy] Add abseil-wrap-unique check
Needs ReviewPublic

Authored by Dosi-Dough on Jan 29 2019, 7:21 PM.

Details

Summary

Based on abseil tip of the week 126: https://abseil.io/tips/126

Created a check which looks for instances of a factory function that uses a non-public constructor and return a std::unique_ptr<T>.

Diff Detail

Event Timeline

Dosi-Dough created this revision.Jan 29 2019, 7:21 PM
Dosi-Dough edited the summary of this revision. (Show Details)Jan 29 2019, 7:21 PM

.DS_Store files clearly should not be there.

Eugene.Zelenko retitled this revision from [clang-tidy] created wrap-unique check to [clang-tidy] Add abseil-wrap-unique check.Jan 30 2019, 11:02 AM
Eugene.Zelenko added inline comments.
clang-tidy/abseil/AbseilTidyModule.cpp
70

Unnecessary empty line.

clang-tidy/abseil/WrapUniqueCheck.cpp
11

Please run Clang-format.

76

Unnecessary empty line.

docs/ReleaseNotes.rst
70

Please use alphabetical order in new checks list.

74

Please use `` to highlight language constructs. Same in documentation.

docs/clang-tidy/checks/abseil-wrap-unique.rst
7

Please synchronize first statement with Release Notes.

25

Please run Clang-format over code snippets.

test/clang-tidy/abseil-wrap-unique.cpp
4

Unnecessary empty line.

32

Unnecessary empty line.

89

Unnecessary empty line.

91

Unnecessary empty lines.

Dosi-Dough marked 11 inline comments as done.

fixed white space/ formatting issues.

removed .DS_Store.

Eugene.Zelenko added inline comments.Jan 30 2019, 6:07 PM
clang-tidy/abseil/WrapUniqueCheck.cpp
80

Please elide braces.

based on feedback:

fixed whitespacing / formatting.

Removed .DS_Store

MyDeveloperDay added inline comments.
clang-tidy/abseil/WrapUniqueCheck.cpp
28

couldn't this be

return (!ArgRef.empty()) ? ArgRef.str() + ")" : "()";

You know I think there is a clang-tidy check for using empty() which was recently extended to handle length() too... I wonder if that would have fired

63

can we declare this when we create it see below?

73

std::string newText = ......

85

clang-tidy file in llvm/tools/clang says

key: readability-identifier-naming.VariableCase
value: CamelCase

this means that local variables like this should be DiagText and NewText (I think!)

86

remove empty declarations until they are created

94

std::string Left = (cons......

95

std::string newText = ....

docs/ReleaseNotes.rst
89

remove double bank line

MyDeveloperDay added inline comments.Jan 31 2019, 2:20 AM
clang-tidy/abseil/WrapUniqueCheck.cpp
55

I'm not sure how expensive getNodeAs is... or if the convention is to do them all at the top of the function

but couldn't some of these getNodeAs go inside the scope where they are used

if (facExpr){

const auto *callExpr = ....
60

same with the 2 above they are only used in the if (cons) scope

MyDeveloperDay added inline comments.Jan 31 2019, 2:22 AM
clang-tidy/abseil/WrapUniqueCheck.h
28

Nit: could this method could be static in the cpp file and not in the header?

This is the clang-tidy revision that mgiht have caught the str().length() > 0 case D56644: [clang-tidy] readability-container-size-empty handle std::string length() adding a a cross reference for that work.

Dosi-Dough marked 11 inline comments as done.

added style and performance refactoring based on code reviews

Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 6:21 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
clang-tidy/abseil/WrapUniqueCheck.cpp
8

License as above

78

we don't put braces on small lines, just

if (Cons->isListInitialization())
  return;
clang-tidy/abseil/WrapUniqueCheck.h
7

this is the wrong license wording now (its changed recently..) check out the other files in trunk

but its something like this....

//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
docs/clang-tidy/checks/abseil-wrap-unique.rst
11

there is normally a newline after a code-block

Dosi-Dough marked 4 inline comments as done.

fixed bracketed return and added updated license

Eugene.Zelenko added inline comments.Mar 27 2019, 5:05 PM
clang-tidy/abseil/WrapUniqueCheck.h
27

Please remove obsolete commend and private keyword.

docs/clang-tidy/checks/abseil-wrap-unique.rst
6

Please separate with empty line.

17

Please use = default;

Dosi-Dough updated this revision to Diff 193421.Apr 2 2019, 8:13 PM
Dosi-Dough marked 2 inline comments as done.

added minor style changes and removed unnecessary inline comments.

Dosi-Dough marked an inline comment as done.

fixed docs and white spacing

Eugene.Zelenko added inline comments.Apr 10 2019, 4:14 PM
clang-tidy/abseil/WrapUniqueCheck.cpp
86

I don't think that you need round brackets around ConsDecl. Same below.

docs/clang-tidy/checks/abseil-wrap-unique.rst
6

Please separate with empty line.

Dosi-Dough marked 3 inline comments as done.

removed parens from turnary, added whitespace.

Eugene.Zelenko added inline comments.Apr 10 2019, 6:15 PM
docs/clang-tidy/checks/abseil-wrap-unique.rst
6

Sorry, if I was not clear. I meant empty line between ================== and statement.

Dosi-Dough marked 2 inline comments as done.

added whitespace to check doc

added white space to check doc.

Dosi-Dough added a comment.EditedApr 17 2019, 4:41 PM

Hi I was wondering if there were any other changes anyone recommends should be made to this revision?

@EricWF @JonasToth I was wondering what other changes I should make to submit my check upstream?