This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add option for disabling AddUsing tweak on some namespaces.
ClosedPublic

Authored by adamcz on Sep 16 2020, 9:19 AM.

Details

Summary

For style guides forbid "using" declarations for namespaces like "std".
With this new config option, AddUsing can be selectively disabled on
those.

Diff Detail

Event Timeline

adamcz created this revision.Sep 16 2020, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 9:19 AM
adamcz requested review of this revision.Sep 16 2020, 9:19 AM

Nice!

clang-tools-extra/clangd/Config.h
66

describe elements more precisely?
They are namespaces with/without leading/trailing ::, and sub-namespaces are implicitly included.

clang-tools-extra/clangd/ConfigCompile.cpp
215

this is a good place to validate and/or normalize (leading/trailing ::)

clang-tools-extra/clangd/ConfigFragment.h
167

Can we describe this positively first?

Namespaces whose members should be fully-qualified, rather than via using declarations or directives.
Affects availability of the "add using declaration" tweak.

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
197

validate it's a namespace, and then call printNamespaceScope (AST.h)?

This handles the right spelling of anonymous/inline NSes.

208

we should do this at config compile time instead I think - conceptually simpler if we store normalized data

210

slightly silly, but can we take a stringref, consume Banned, and verify that the result is empty or begins with ::?

Avoids a silly string copy, explicitly handles the sub-namespace case, and is just more typical of the string code we write.

adamcz updated this revision to Diff 292521.Sep 17 2020, 8:44 AM
adamcz marked 5 inline comments as done.

addressed review comments

clang-tools-extra/clangd/ConfigFragment.h
167

I copied the comment from the other place and added the bit about tweak. WDYT?

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
197

It's already guaranteed to be a namespace, that's checked before calling this function.

sammccall accepted this revision.Sep 18 2020, 1:02 AM

Thanks!
Can we add a simple test to TweakTests (I should really split up that file) and to ConfigCompileTests?

(BTW, do you think we should merge Config{Compile,YAML}Tests?

clang-tools-extra/clangd/Config.h
70

should just be "without leading ::" now I think?

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
200

not needed - printNamespaceScope never starts with ::

201

inlining this into the if (consume_front(Banned) && consume_front(::)) might be clearer, up to you

This revision is now accepted and ready to land.Sep 18 2020, 1:02 AM
adamcz updated this revision to Diff 292786.Sep 18 2020, 7:31 AM
adamcz marked 2 inline comments as done.

addressed review comments

clang-tools-extra/clangd/Config.h
70

What I was trying to say here is that both ::foo::bar and foo::bar are allowed, but:

using foo;
bar

is not. Basically I'm trying to explain that what we mean by fully qualified is the no-using-shortcuts, not always-start-with-::

This isn't about what's stored in the vector, but rather what this options means.

This revision was landed with ongoing or failed builds.Sep 18 2020, 8:01 AM
This revision was automatically updated to reflect the committed changes.