Crank up static analysis audit #2607

miniksa · 2019-08-29T17:31:34Z

Summary of the Pull Request

Brings audit mode up to full warning speed and fixes relevant errors in the four active projects.

PR Checklist

Closes #xxx
I'm an employee
Tests added/passed - This is a test of sorts
Requires documentation to be updated - Yes, should probably mention audits somewhere in docs.
I'm a core contributor

Detailed Description of the Pull Request / Additional comments

…peReader to use std::array so we can use .at and move some pointers to iterators.

…r use safe math)

…except

src/types/UTF8OutPipeReader.cpp

src/types/inc/UTF8OutPipeReader.hpp

…licy. Also reinstitute C6001 and C6011 because they're not actually a part of the 'core checks' and they're goodness we had before I turned them off at the beginning of this series.

…e time.

…ouldn't need to, internal methods only.

…k for null as a defense against a bad QI implementation.

…essary span refs).

src/buffer/out/OutputCellView.cpp

DHowett-MSFT · 2019-09-05T21:27:18Z

src/buffer/out/cursor.cpp

 {
    _RedrawCursor();
-    _cPosition.X = (SHORT)NewX;
+    _cPosition.X = gsl::narrow<SHORT>(NewX);


this is marked noexcept but i thought narrow was the throwing one

Urgh, it's actually weird and I should probably undo the one I just did earlier. gsl::narrow would have been detected by the static analysis as throwing and it would have errored out if it was a problem.

The trick is that I believe the implementation of narrow causes a Fail Fast and isn't actually throwing anything. Since that's not going to come back or in any way continue, it isn't technically violating the noexcept marking...

src/buffer/out/textBuffer.cpp

DHowett-MSFT · 2019-09-05T21:28:40Z

src/buffer/out/textBuffer.cpp

 {
-    return static_cast<UINT>(_storage.size());
+    return gsl::narrow<UINT>(_storage.size());


(same as above)

src/buffer/out/textBufferCellIterator.cpp

src/buffer/out/textBufferTextIterator.cpp

src/renderer/dx/CustomTextRenderer.cpp

src/renderer/dx/DxRenderer.cpp

src/renderer/dx/DxRenderer.hpp

DHowett-MSFT · 2019-09-05T22:59:34Z

src/types/CodepointWidthDetector.cpp

+// Routine Description:
+// - Constructs an instance of the CodepointWidthDetector class
+CodepointWidthDetector::CodepointWidthDetector() noexcept :
+    _fallbackCache{},


both of these should automatically get default-constructed for us, and we should be able to default our constructor.

I think I did that and it carped so I did this.

src/types/CodepointWidthDetector.cpp

src/types/KeyEvent.cpp

src/types/ScreenInfoUiaProviderBase.cpp

src/types/inc/CodepointWidthDetector.hpp

src/types/inc/utils.hpp

zadjii-msft

Yea this seems good to me. I have some minor notes, feel free to fix if you want.

src/buffer/out/OutputCell.cpp

src/buffer/out/textBuffer.cpp

src/renderer/dx/CustomTextRenderer.cpp

src/types/convert.cpp

src/renderer/dx/DxRenderer.cpp

…an extraneous local.

miniksa added 12 commits August 29, 2019 10:27

Make warnings errors for static analysis.

1989eb9

C26446, Use .at instead of array indices

65dec36

[Complex] C26446, Use .at instead of array indices - Reword UTF8OutPi…

23897b1

…peReader to use std::array so we can use .at and move some pointers to iterators.

C26477, don't use 0 or NULL, use nullptr.

bd2d5dd

C26496, mark const if it's never written after creation

b33a598

C26493, no C-style casts.

c63289b

C26435, choose one of virtual, override, or final

a381f6a

C26472, no static_cast for arithmetic conversions. narrow or narrow_cast

8ea7401

C26433, overrides should be explicit.

50e2d0c

C26451, promote before arithmetic if storing in larger result size (o…

8579d89

…r use safe math)

C26481, don't use pointer arithemetic. use span.

8c3a629

C26447,C26440 - is noexcept but can throw or doesn't throw but not no…

4f1157c

…except

german-one reviewed Aug 30, 2019

View reviewed changes

src/types/UTF8OutPipeReader.cpp Outdated Show resolved Hide resolved

german-one reviewed Aug 30, 2019

View reviewed changes

src/types/UTF8OutPipeReader.cpp Outdated Show resolved Hide resolved

german-one reviewed Aug 30, 2019

View reviewed changes

src/types/UTF8OutPipeReader.cpp Outdated Show resolved Hide resolved

german-one reviewed Aug 30, 2019

View reviewed changes

src/types/inc/UTF8OutPipeReader.hpp Outdated Show resolved Hide resolved

miniksa added 14 commits September 3, 2019 08:46

C26429, symbols not tested for nullness.

30e8e7f

C26474, don't use static_cast when an implicit cast is acceptable.

cdfbf8f

C26466, disable dynamic_cast rule because we're not RTTI due to OS po…

230e7f4

…licy. Also reinstitute C6001 and C6011 because they're not actually a part of the 'core checks' and they're goodness we had before I turned them off at the beginning of this series.

C26485, refactor to avoid array-to-pointer decay.

7d4096b

C26473, do not cast pointer back to the same type.

81ab580

C26494, uninitalized local variables

d5d7cf4

C26462, const local variables that are unchanged.

bbdfdf9

C26445, wstring_view byref may indicate a lifetime issue

b180406

C26497, use constexpr for functions that could be evaluated at compil…

c956913

…e time.

C26429, mark gsl::not_null on places where we don't test for null (sh…

594dca9

…ouldn't need to, internal methods only.

C26430, not tested for nullness on all paths. I will just always chec…

45e5993

…k for null as a defense against a bad QI implementation.

C26414, don't use smart pointers for locals

9678dd8

C26460, use const on params that are unchanged (and remove some unnec…

dd49c3e

…essary span refs).

forgot all return paths to _FillRectangle.

d8bc94f