Remove ATL dependencies (#676) #719

ysc3839 · 2019-05-12T16:33:13Z

Related discussion #676.

msftclas · 2019-05-12T16:33:25Z

All CLA requirements met.

src/tsf/StructureArray.h

src/tsf/ConsoleTSF.cpp

binarycrusader · 2019-05-12T23:38:40Z

src/tsf/TfEditses.cpp

    // set GUID_PROP_CONIME_TRACKCOMPOSITION
    //
-    CComPtr<ITfProperty> PropertyTrackComposition;
+    wil::com_ptr_nothrow<ITfProperty> PropertyTrackComposition;


I think the application of com_ptr_nothrow is correct, but I was surprised to see that all of these use the non-throwing version of wil's com_ptr since I assumed at least a few cases should throw an exception instead.

Was the new use verified to ensure that the nothrow variant matches the previous behavior in all cases?

I took a look at atlcomcli.h and found operator* in CComPtrBase throws expection.

T& operator*() const { ATLENSURE(p!=NULL); return *p; }

#define ATLENSURE(expr) ATLENSURE_THROW(expr, E_FAIL)

So here we can use wil::com_ptr instead of wil::com_ptr_nothrow?

I would wait for one of the Terminal team members to respond before changing this. But my intuition is that it was correct to use com_ptr_nothrow for the query cases, but the others should be com_ptr.

Actually we were still discussing in the issue if com_ptr_nothrow is actually needed in TSF or not. I'd say that it isn't, exceptions can come from query functions but we actually need the "try" versions.

I took a look at atlcomcli.h and found operator* in CComPtrBase throws expection.

Yes but wil's com_ptr does not. Besides, I don't think I've seen any uses of this particular operator.

The CEditSessionObject::_GetTextAndAttributeGapRange function is already a little broken in that we were calling std::wstring.append here which I believe can technically throw in memory situations and we weren't catching it.

What I'd do here is wrap the entire function body in:

try { } CATCH_RETURN(); return S_OK;

And just use the throwable versions of wil::com_ptr and continue to use the std::wstring.append inside of that.

That will ensure that any exception that might be thrown now or in future refactorings will be appropriately caught and HRESULT-ified for the COM caller.

I added try blocks only for CompGuid.insert and CompStr.append.

binarycrusader · 2019-05-12T23:39:47Z

I think these changes are basically correct, but I'm going to defer formal review to the Terminal project team.

This looks like excellent work overall.

ysc3839 · 2019-05-13T15:02:16Z

The last project uses ATL is TerminalParser.Fuzzer. It uses CStringA/W for AppendFormat function which is absent in STL.
So should we implement our own AppendFormat for std::string?

src/tsf/ConsoleTSF.cpp

zadjii-msft

Seems reasonable, but I want to make sure the rest of the team gets their eyes on it.

src/tsf/TfCtxtComp.h

src/tsf/TfCatUtil.cpp

src/tsf/TfDispAttr.cpp

src/tsf/TfEditses.cpp

src/tsf/TfTxtevCb.cpp

mikedn · 2019-05-13T16:23:37Z

src/tsf/TfConvArea.cpp

    std::basic_string_view<WORD> colorArray(colors.data(), colors.size());

-    return ImeComposeData(text, attributes, colorArray);
+    return ImeComposeData(CompStr, attributes, colorArray);


Would be nicer to rename the parameter to text instead

I don't think we should rename it. It has it's meaning.

src/tsf/TfEditses.cpp

miniksa · 2019-05-13T16:48:12Z

The last project uses ATL is TerminalParser.Fuzzer. It uses CStringA/W for AppendFormat function which is absent in STL.
So should we implement our own AppendFormat for std::string?

I don't like the idea of implementing our own AppendFormat because I swear there has to be a solution out there to this problem in STL. Perhaps using a std::stringstream to build up the concatenation in fuzzing_logic.h?

Otherwise, the only other thing I can think of is to go back to old school CRT vsprintf.

mikedn · 2019-05-13T17:06:13Z

Most of the fuzzer needs are pretty simple, you can use just std::to_string and string concatenation. The only problem is that in a few cases the format string specifies a precision (e.g. %02d) and to_string doesn't handle that. A custom std::string ToString(T value, int precision) implemented using ostringstream should do the job.

miniksa

Overall, this is exactly what I was looking for when I made the issue. Thank you so much for stepping up here. Just cleanup the outstanding threads and we're good to go.

src/tsf/TfCtxtComp.h

src/tsf/precomp.h

src/tsf/TfConvArea.h

ysc3839 · 2019-05-14T20:29:13Z

@miniksa Yes.

miniksa · 2019-05-14T21:37:11Z

@ysc3839 sure go for it.

src/terminal/parser/ft_fuzzer/string_helper.h

ysc3839 · 2019-05-15T19:53:03Z

fuzzing_directed.h uses CComAllocator:

terminal/src/terminal/parser/ft_fuzzer/fuzzing_directed.h

Lines 28 to 30 in a12521f

    
           #ifndef __FUZZING_ALLOCATOR 
        
           #define __FUZZING_ALLOCATOR CComAllocator 
        
           #endif

And there's comments about it:

terminal/src/terminal/parser/ft_fuzzer/fuzzing_directed.h

Lines 329 to 342 in a12521f

    
           // The CFuzzArray class is designed to allow fuzzing of element 
        
           // arrays, potentially reallocating a fuzzed version of the array 
        
           // that is either larger or smaller than the template buffer.  Whether 
        
           // reallocation is possible (or an appropriate fuzzing strategy) is 
        
           // dependent on the scenario and must be determined by the person 
        
           // designing the fuzz map.  If reallocation is going to be used, the 
        
           // _Alloc class must specify the appropriate allocator that corresponds 
        
           // to the code that is being fuzzed.  For example, if all allocations 
        
           // are made using new/delete, the default CComAllocator is not 
        
           // appropriate and should be changed to CCRTAllocator.  Adding new 
        
           // allocator classes is as easy as writing a new class that supports 
        
           // Allocate/Free/Reallocate, see documentation for CComAllocator. 
        
           template <class _Alloc, typename _Type1, typename _Type2, typename... _Args> 
        
           class CFuzzArray : public CFuzzBase

So can we just copy CCRTAllocator implementation?

class CCRTAllocator
{
public:
	_Ret_maybenull_ _Post_writable_byte_size_(nBytes) _ATL_DECLSPEC_ALLOCATOR static void* Reallocate(
		_In_ void* p,
		_In_ size_t nBytes) throw()
	{
		return realloc(p, nBytes);
	}

	_Ret_maybenull_ _Post_writable_byte_size_(nBytes) _ATL_DECLSPEC_ALLOCATOR static void* Allocate(_In_ size_t nBytes) throw()
	{
		return malloc(nBytes);
	}

	static void Free(_In_ void* p) throw()
	{
		free(p);
	}
};

miniksa · 2019-05-16T18:11:22Z

So can we just copy CCRTAllocator implementation?

Yeah, I'd just do that and rename it slightly or put it into a namespace of our own so it never conflicts.

src/terminal/parser/ft_fuzzer/fuzzing_directed.h

src/terminal/parser/ft_fuzzer/string_helper.h

ysc3839 · 2019-05-20T17:46:12Z

@miniksa Any update?

miniksa · 2019-05-21T16:26:26Z

@ysc3839, sorry, I have been swamped. There's too many of you and too few of me. I'll look.

miniksa · 2019-05-21T16:28:12Z

OK, I double checked. I'm good with this. I'm trying to get another FTE to sign off.

ghost · 2019-05-21T16:28:25Z

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

zadjii-msft · 2019-05-21T16:49:19Z

@msftbot hold this pr for the next 2 hours

ghost · 2019-05-21T16:49:57Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Tue, 21 May 2019 18:49:57 GMT, which is in 2 hours

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

zadjii-msft

I'm giving @DHowett-MSFT a chance to get his eyes on this PR, but I think it's fine

DHowett-MSFT

This is great. Thank you!

EydenVillanueva approved these changes May 12, 2019

View reviewed changes

binarycrusader reviewed May 12, 2019

View reviewed changes

src/tsf/StructureArray.h Outdated Show resolved Hide resolved

binarycrusader reviewed May 12, 2019

View reviewed changes

src/tsf/ConsoleTSF.cpp Outdated Show resolved Hide resolved

binarycrusader reviewed May 12, 2019

View reviewed changes

ysc3839 force-pushed the rm-atl-deps branch from 1822e71 to 13311ab Compare May 13, 2019 04:44