Differential D15532 Diff 41944 plugins/astyle/astyle_plugin.cpp

Changeset View

Standalone View

View Options

plugins/astyle/astyle_plugin.cpp

Show All 27 Lines
28#include <interfaces/isourceformattercontroller.h> 28​#include <interfaces/isourceformattercontroller.h>
29 29​
30#include "astyle_formatter.h" 30​#include "astyle_formatter.h"
31#include "astyle_stringiterator.h" 31​#include "astyle_stringiterator.h"
32#include "astyle_preferences.h" 32​#include "astyle_preferences.h"
33#include <KLocalizedString> 33​#include <KLocalizedString>
34#include <QUrl> 34​#include <QUrl>
35 35​
36​static const char *formattingCxxSample =
pinoUnsubmitted
Done

static const char foo[] = "...", which is more efficient (makes the whole string const)

pino: `static const char foo[] = "..."`, which is more efficient (makes the whole string const)
rjvbbAuthorUnsubmitted
Not Done

And there I was thinking the days were gone where foo[] and *foo were ever so subtly different... :)

rjvbb: And there I was thinking the days were gone where foo[] and *foo were ever so subtly different..
kossebauUnsubmitted
Not Done

For what I know, the days are still on-going due to this:

static const char * foo = "foo";

is a non-const pointer to a const char array. You can do (accidentally)

foo = "bar";

and the compiler will not see this runs against the (usual) intent.

It is also one indirection more (unless the compiler is sure enough to be able to optimize this out), as foo is a symbol for a data that is a pointer, which holds the address of the char array.
Where with

static const char foo[] = "foo";

foo is a symbol for a data that is the char array.

kossebau: For what I know, the days are still on-going due to this: ``` static const char * foo = "foo"…
aaronpuchertUnsubmitted
Not Done

The advice is mostly important for exported symbols, see Ulrich Drepper: How To Write Shared Libraries, section 2.4. For static (internal linkage) variables the compiler knows all references to the variable, so it can remove the indirection and see that it is const (if it is).

Nevertheless, it obviously doesn't hurt to get used to writing char[], because sometimes it does matter.

By the way, even more modern would be static constexpr char[].

aaronpuchert: The advice is mostly important for exported symbols, see Ulrich Drepper: [How To Write Shared…
kossebauUnsubmitted
Not Done

Not yet firm with constexpr myself, so curious to learn what would one gain from using static constexpr char[] here? What exactly would be buildtime evaluated?
I think I have seen constexpr already in KDevelop code, so if that is a better version, would be interested to improve all similar places.

kossebau: Not yet firm with constexpr myself, so curious to learn what would one gain from using `static…
aaronpuchertUnsubmitted
Not Done

Basically constexpr forces compile-time evaluation, which means that

  1. The variable will (almost certainly) land in a read-only segment.
  2. There is no dynamic initialization order fiasco.

In this case it wouldn't make much of a difference, since it is a compile-time constant and the compiler will treat is as such. But with constexpr you get a compilation error if it isn't.

aaronpuchert: Basically `constexpr` forces compile-time evaluation, which means that # The variable will…
37​ "void func(){\n"
38​ "\tif(isFoo(a,b))\n"
39​ "\tbar(a,b);\n"
40​ "if(isFoo)\n"
41​ "\ta=bar((b-c)*a,*d--);\n"
42​ "if( isFoo( a,b ) )\n"
43​ "\tbar(a, b);\n"
44​ "if (isFoo) {isFoo=false;cat << isFoo <<endl;}\n"
45​ "if(isFoo)DoBar();if (isFoo){\n"
46​ "\tbar();\n"
47​ "}\n"
48​ "\telse if(isBar()){\n"
49​ "\tannotherBar();\n"
50​ "}\n"
51​ "int var = 1;\n"
52​ "int *ptr = &var;\n"
53​ "int& ref = i;\n"
54​ "\n"
55​ "QList<int>::const_iterator it = list.begin();\n"
56​ "}\n"
57​ "namespace A {\n"
58​ "namespace B {\n"
59​ "class someClass {\n"
60​ "void foo() {\n"
61​ " if (true) {\n"
62​ " func();\n"
63​ " } else {\n"
64​ " // bla\n"
65​ " }\n"
66​ "}\n"
67​ "};\n"
68​ "}\n"
69​ "}\n";
70​
71​static const char *formattingObjCSample =
pinoUnsubmitted
Done

ditto

pino: ditto
72​ "void func(){\n"
73​ "\tif(isFoo(a,b))\n"
74​ "\tbar(a,b);\n"
75​ "if(isFoo)\n"
76​ "\ta=bar((b-c)*a,*d--);\n"
77​ "if( isFoo( a,b ) )\n"
78​ "\tbar(a, b);\n"
79​ "if (isFoo) {isFoo=false;cat << isFoo <<endl;}\n"
80​ "if(isFoo)DoBar();if (isFoo){\n"
81​ "\tbar();\n"
82​ "}\n"
83​ "\telse if(isBar()){\n"
84​ "\tannotherBar();\n"
85​ "}\n"
86​ "int var = 1;\n"
87​ "int *ptr = &var;\n"
88​ "\n"
89​ "}\n"
90​ "@implementation someClass \n"
91​ "+ (someClass*) someClassWithFoo:(int)foo\n"
92​ "{\n"
93​ " someClass *this;\n"
94​ " if (foo) {\n"
95​ " this = [[someClass alloc] initWith:foo];\n"
96​ " } else {\n"
97​ " // bla\n"
98​ " }\n"
99​ " return self;\n"
100​ "}\n"
101​ "@end\n";
102​
103​static const char *indentingCxxSample =
pinoUnsubmitted
Done

ditto

pino: ditto
104​ "#define foobar(A)\\\n"
105​ "{Foo();Bar();}\n"
106​ "#define anotherFoo(B)\\\n"
107​ "return Bar()\n"
108​ "\n"
109​ "namespace Bar\n"
110​ "{\n"
111​ "class Foo\n"
112​ "{public:\n"
113​ "Foo();\n"
114​ "virtual ~Foo();\n"
115​ "};\n"
116​ "void bar(int foo)\n"
117​ "{\n"
118​ "switch (foo)\n"
119​ "{\n"
120​ "case 1:\n"
121​ "a+=1;\n"
122​ "break;\n"
123​ "case 2:\n"
124​ "{\n"
125​ "a += 2;\n"
126​ " break;\n"
127​ "}\n"
128​ "}\n"
129​ "if (isFoo)\n"
130​ "{\n"
131​ "bar();\n"
132​ "}\n"
133​ "else\n"
134​ "{\n"
135​ "anotherBar();\n"
136​ "}\n"
137​ "}\n"
138​ "int foo()\n"
139​ "\twhile(isFoo)\n"
140​ "\t\t{\n"
141​ "\t\t\t// ...\n"
142​ "\t\t\tgoto error;\n"
143​ "\t\t/* .... */\n"
144​ "\t\terror:\n"
145​ "\t\t\t//...\n"
146​ "\t\t}\n"
147​ "\t}\n"
148​ "fooArray[]={ red,\n"
149​ "\tgreen,\n"
150​ "\tdarkblue};\n"
151​ "fooFunction(barArg1,\n"
152​ "\tbarArg2,\n"
153​ "\tbarArg3);\n"
154​ "struct foo{ int bar() {} };\n";
155​
156​static const char *indentingObjCSample =
pinoUnsubmitted
Done

ditto

pino: ditto
157​ "#import <objc/Object.h>\n"
158​ "\n"
159​ "#define foobar(A)\\\n"
160​ "\t{Foo();Bar();}\n"
161​ "#define anotherFoo(B)\\\n"
162​ "\treturn Bar()\n"
163​ "\n"
164​ "@interface Foo : Bar {\n"
165​ "@private\n"
166​ "\tid Baz;\n"
167​ "}\n"
168​ "- (void) init;\n"
169​ "- (NSString*) description;\n"
170​ "@property (retain) id Baz;\n"
171​ "@end\n"
172​ "\n"
173​ "@interface Foo (Bar)\n"
174​ "- (void)bar:(int) foo;\n"
175​ "@end\n"
176​ "\n"
177​ "@implementation Foo (Bar)\n"
178​ "\n"
179​ "- (void) bar:(int) foo\n"
180​ "{\n"
181​ "\tswitch (foo) {\n"
182​ "case 1:\n"
183​ "a += 1;\n"
184​ "break;\n"
185​ "case 2: {\n"
186​ "a += 2;\n"
187​ "break;\n"
188​ "}\n"
189​ "}\n"
190​ "if (isFoo) {\n"
191​ "bar();\n"
192​ "} else {\n"
193​ "[anotherBar withFoo:self];\n"
194​ "}\n"
195​ "}\n"
196​ "\n"
197​ "@end\n"
198​ "int foo()\n"
199​ "while (isFoo)\n"
200​ "{\n"
201​ "\t// ...\n"
202​ "\tgoto error;\n"
203​ "\t/* .... */\n"
204​ "error:\n"
205​ "\t//...\n"
206​ "}\n"
207​ "\n"
208​ "fooArray[] = { red,\n"
209​ "\tgreen,\n"
210​ "\tdarkblue};\n"
211​ "fooFunction(barArg1,\n"
212​ "\tbarArg2,\n"
213​ "\tbarArg3);\n"
214​ "struct foo { int bar() {} };\n";
215​
36using namespace KDevelop; 216​using namespace KDevelop;
37 217​
38K_PLUGIN_FACTORY_WITH_JSON(AStyleFactory, "kdevastyle.json", registerPlugin<AStylePlugin>();) 218​K_PLUGIN_FACTORY_WITH_JSON(AStyleFactory, "kdevastyle.json", registerPlugin<AStylePlugin>();)
39 219​
40AStylePlugin::AStylePlugin(QObject *parent, const QVariantList&) 220​AStylePlugin::AStylePlugin(QObject *parent, const QVariantList&)
41 : IPlugin(QStringLiteral("kdevastyle"), parent) 221​ : IPlugin(QStringLiteral("kdevastyle"), parent)
42 , m_formatter(new AStyleFormatter()) 222​ , m_formatter(new AStyleFormatter())
43{ 223​{
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Line(s)270{
91 auto style = ICore::self()->sourceFormatterController()->styleForUrl(url, mime); 271​ auto style = ICore::self()->sourceFormatterController()->styleForUrl(url, mime);
92 return formatSourceWithStyle(style, text, url, mime, leftContext, rightContext); 272​ return formatSourceWithStyle(style, text, url, mime, leftContext, rightContext);
93} 273​}
94 274​
95static SourceFormatterStyle::MimeList supportedMimeTypes() 275​static SourceFormatterStyle::MimeList supportedMimeTypes()
96{ 276​{
97 return { 277​ return {
98 {QStringLiteral("text/x-c++src"), QStringLiteral("C++")}, 278​ {QStringLiteral("text/x-c++src"), QStringLiteral("C++")},
279​ {QStringLiteral("text/x-csrc"), QStringLiteral("C")},
99 {QStringLiteral("text/x-chdr"), QStringLiteral("C")}, 280​ {QStringLiteral("text/x-chdr"), QStringLiteral("C")},
100 {QStringLiteral("text/x-c++hdr"), QStringLiteral("C++")}, 281​ {QStringLiteral("text/x-c++hdr"), QStringLiteral("C++")},
101 {QStringLiteral("text/x-csrc"), QStringLiteral("C")}, ​
102 {QStringLiteral("text/x-java"), QStringLiteral("Java")}, 282​ {QStringLiteral("text/x-java"), QStringLiteral("Java")},
103 {QStringLiteral("text/x-csharp"), QStringLiteral("C#")}, 283​ {QStringLiteral("text/x-csharp"), QStringLiteral("C#")},
284​ {QStringLiteral("text/x-objcsrc"), QStringLiteral("ObjC")},
kossebauUnsubmitted
Done

What about "ObjC" -> "Objective-C"? That is the id also used for X-KDevelop-Languages, would be good to be consistent.
Also is this the string currently used for display, so not using an abbreviation might be better.

kossebau: What about "ObjC" -> "Objective-C"? That is the id also used for X-KDevelop-Languages, would be…
rjvbbAuthorUnsubmitted
Not Done

Thanks, that's a nice reminder there a few more places where changes are required and for which I have D15530 up!

rjvbb: Thanks, that's a nice reminder there a few more places where changes are required and for which…
kossebauUnsubmitted
Not Done

And as I meanwhile found, using "Objective-C" and "Objective-C++" here is also required, as this is by chance also the id used by the syntax highlighting mode for the respective highlighting rules.

kossebau: And as I meanwhile found, using "Objective-C" and "Objective-C++" here is also required, as…
285​ {QStringLiteral("text/x-objc++src"), QStringLiteral("ObjC++")},
286​ {QStringLiteral("text/x-objchdr"), QStringLiteral("ObjC")},
104 }; 287​ };
105} 288​}
106 289​
107SourceFormatterStyle predefinedStyle(const QString& name, const QString& caption = QString()) 290​SourceFormatterStyle predefinedStyle(const QString& name, const QString& caption = QString())
108{ 291​{
109 SourceFormatterStyle st = SourceFormatterStyle( name ); 292​ SourceFormatterStyle st = SourceFormatterStyle( name );
110 st.setCaption( caption.isEmpty() ? name : caption ); 293​ st.setCaption( caption.isEmpty() ? name : caption );
111 AStyleFormatter fmt; 294​ AStyleFormatter fmt;
Show All 24 Lines
136 319​
137SettingsWidget* AStylePlugin::editStyleWidget(const QMimeType& mime) const 320​SettingsWidget* AStylePlugin::editStyleWidget(const QMimeType& mime) const
138{ 321​{
139 AStylePreferences::Language lang = AStylePreferences::CPP; 322​ AStylePreferences::Language lang = AStylePreferences::CPP;
140 if(mime.inherits(QStringLiteral("text/x-java"))) 323​ if(mime.inherits(QStringLiteral("text/x-java")))
141 lang = AStylePreferences::Java; 324​ lang = AStylePreferences::Java;
142 else if(mime.inherits(QStringLiteral("text/x-csharp"))) 325​ else if(mime.inherits(QStringLiteral("text/x-csharp")))
143 lang = AStylePreferences::CSharp; 326​ lang = AStylePreferences::CSharp;
327​ else if(mime.inherits(QStringLiteral("text/x-objcsrc")) || mime.inherits(QStringLiteral("text/x-objc++src"))) {
kossebauUnsubmitted
Done

While adding this line, already add a space between if and (.

kossebau: While adding this line, already add a space between if and (.
rjvbbAuthorUnsubmitted
Not Done

I take it you meant fixing the whitespace for the entire method(?)

rjvbb: I take it you meant fixing the whitespace for the entire method(?)
kossebauUnsubmitted
Not Done

Actually I only meant the lines you are touching, sorry for being unclear.

kossebau: Actually I only meant the lines you are touching, sorry for being unclear.
328​ // x-objc++src *should* inherit x-objcsrc but that is not always the case in practice
329​ lang = AStylePreferences::ObjC;
330​ }
144 return new AStylePreferences(lang); 331​ return new AStylePreferences(lang);
145} 332​}
146 333​
147QString AStylePlugin::previewText(const SourceFormatterStyle& /*style*/, const QMimeType& /*mime*/) const 334​QString AStylePlugin::previewText(const SourceFormatterStyle& /*style*/, const QMimeType& mime) const
148{ 335​{
149 return QLatin1String("// Indentation\n") + indentingSample() + QLatin1String("\t// Formatting\n") 336​ QString preview = QLatin1String("// Indentation\n");
150 + formattingSample(); 337​ AStylePreferences::Language lang;
338​ if (mime.inherits(QStringLiteral("text/x-objcsrc")) || mime.inherits(QStringLiteral("text/x-objc++src"))) {
339​ lang = AStylePreferences::ObjC;
340​ } else {
341​ lang = AStylePreferences::CPP;
342​ }
343​ // TODO: add previews for the other supported languages
344​ preview = indentingSample(lang);
kossebauUnsubmitted
Done

Please keep this one single string calculation:

return
    QLatin1String("// Indentation\n") +
    indentingSample(lang) +
    QLatin1String("\t// Formatting\n") +
    formattingSample(lang);
kossebau: Please keep this one single string calculation: ``` return QLatin1String("//…
345​ preview += QLatin1String("\t// Formatting\n");
346​ preview += formattingSample(lang);
347​ return preview;
151} 348​}
152 349​
153AStylePlugin::Indentation AStylePlugin::indentation(const QUrl& url) const 350​AStylePlugin::Indentation AStylePlugin::indentation(const QUrl& url) const
154{ 351​{
155 // Call formatSource first, to initialize the m_formatter data structures according to the URL 352​ // Call formatSource first, to initialize the m_formatter data structures according to the URL
156 formatSource(QString(), url, QMimeDatabase().mimeTypeForUrl(url), QString(), QString()); 353​ formatSource(QString(), url, QMimeDatabase().mimeTypeForUrl(url), QString(), QString());
157 354​
158 Indentation ret; 355​ Indentation ret;
159 356​
160 ret.indentWidth = m_formatter->option(QStringLiteral("FillCount")).toInt(); 357​ ret.indentWidth = m_formatter->option(QStringLiteral("FillCount")).toInt();
161 358​
162 QString s = m_formatter->option(QStringLiteral("Fill")).toString(); 359​ QString s = m_formatter->option(QStringLiteral("Fill")).toString();
163 if(s == QLatin1String("Tabs")) 360​ if(s == QLatin1String("Tabs"))
164 { 361​ {
165 // Do tabs-only indentation 362​ // Do tabs-only indentation
166 ret.indentationTabWidth = ret.indentWidth; 363​ ret.indentationTabWidth = ret.indentWidth;
167 }else{ 364​ }else{
168 // Don't use tabs at all 365​ // Don't use tabs at all
169 ret.indentationTabWidth = -1; 366​ ret.indentationTabWidth = -1;
170 } 367​ }
171 368​
172 return ret; 369​ return ret;
173} 370​}
174 371​
175QString AStylePlugin::formattingSample() 372​QString AStylePlugin::formattingSample(AStylePreferences::Language lang)
176{ 373​{
177 return QStringLiteral( 374​ switch (lang) {
178 "void func(){\n" 375​ case AStylePreferences::ObjC:
179 "\tif(isFoo(a,b))\n" 376​ return QLatin1String(formattingObjCSample);
180 "\tbar(a,b);\n" 377​ break;
kfunkUnsubmitted
Done

No break; needed after return.

kfunk: No `break;` needed after return.
rjvbbAuthorUnsubmitted
Not Done

I know, I always add them anyway in my own code (just like I always add a default: with a break). Saves headscratching later on when you refactor and miss a missing break...

rjvbb: I know, I always add them anyway in my own code (just like I always add a default: with a…
aaronpuchertUnsubmitted
Not Done

We could activate -Wunreachable-code so the compiler tells you where break isn't needed. With -Wfallthrough the compiler can detect unintended fallthroughs, so you wouldn't need to worry about a missing break either.

aaronpuchert: We could activate `-Wunreachable-code` so the compiler tells you where `break` isn't needed.
181 "if(isFoo)\n" 378​ default:
182 "\ta=bar((b-c)*a,*d--);\n" 379​ return QLatin1String(formattingCxxSample);
183 "if( isFoo( a,b ) )\n" 380​ break;
kfunkUnsubmitted
Done

Add a Q_UNREACHABLE(); at the end of this function, as a small debugging aid.

kfunk: Add a `Q_UNREACHABLE();` at the end of this function, as a small debugging aid.
184 "\tbar(a, b);\n" 381​ }
185 "if (isFoo) {isFoo=false;cat << isFoo <<endl;}\n" ​
186 "if(isFoo)DoBar();if (isFoo){\n" ​
187 "\tbar();\n" ​
188 "}\n" ​
189 "\telse if(isBar()){\n" ​
190 "\tannotherBar();\n" ​
191 "}\n" ​
192 "int var = 1;\n" ​
193 "int *ptr = &var;\n" ​
194 "int& ref = i;\n" ​
195 "\n" ​
196 "QList<int>::const_iterator it = list.begin();\n" ​
197 "}\n" ​
198 "namespace A {\n" ​
199 "namespace B {\n" ​
200 "class someClass {\n" ​
201 "void foo() {\n" ​
202 " if (true) {\n" ​
203 " func();\n" ​
204 " } else {\n" ​
205 " // bla\n" ​
206 " }\n" ​
207 "}\n" ​
208 "};\n" ​
209 "}\n" ​
210 "}\n" ​
211 ); ​
212} 382​}
213 383​
214QString AStylePlugin::indentingSample() 384​QString AStylePlugin::indentingSample(AStylePreferences::Language lang)
215{ 385​{
216 return QStringLiteral( 386​ switch (lang) {
217 "#define foobar(A)\\\n" 387​ case AStylePreferences::ObjC:
218 "{Foo();Bar();}\n" 388​ return QLatin1String(indentingObjCSample);
219 "#define anotherFoo(B)\\\n" 389​ break;
220 "return Bar()\n" 390​ default:
221 "\n" 391​ return QLatin1String(indentingCxxSample);
222 "namespace Bar\n" 392​ break;
223 "{\n" 393​ }
224 "class Foo\n" ​
225 "{public:\n" ​
226 "Foo();\n" ​
227 "virtual ~Foo();\n" ​
228 "};\n" ​
229 "void bar(int foo)\n" ​
230 "{\n" ​
231 "switch (foo)\n" ​
232 "{\n" ​
233 "case 1:\n" ​
234 "a+=1;\n" ​
235 "break;\n" ​
236 "case 2:\n" ​
237 "{\n" ​
238 "a += 2;\n" ​
239 " break;\n" ​
240 "}\n" ​
241 "}\n" ​
242 "if (isFoo)\n" ​
243 "{\n" ​
244 "bar();\n" ​
245 "}\n" ​
246 "else\n" ​
247 "{\n" ​
248 "anotherBar();\n" ​
249 "}\n" ​
250 "}\n" ​
251 "int foo()\n" ​
252 "\twhile(isFoo)\n" ​
253 "\t\t{\n" ​
254 "\t\t\t// ...\n" ​
255 "\t\t\tgoto error;\n" ​
256 "\t\t/* .... */\n" ​
257 "\t\terror:\n" ​
258 "\t\t\t//...\n" ​
259 "\t\t}\n" ​
260 "\t}\n" ​
261 "fooArray[]={ red,\n" ​
262 "\tgreen,\n" ​
263 "\tdarkblue};\n" ​
264 "fooFunction(barArg1,\n" ​
265 "\tbarArg2,\n" ​
266 "\tbarArg3);\n" ​
267 "struct foo{ int bar() {} };\n" ​
268 ); ​
269} 394​}
270 395​
271#include "astyle_plugin.moc" 396​#include "astyle_plugin.moc"